Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7053009:
Add chrome://quota-internals/ resources (Closed)
|
Created:
9 years, 7 months ago by tzik Modified:
9 years, 6 months ago CC:
chromium-reviews, arv (Not doing code reviews), pam+watch_chromium.org Visibility:
Public. |
DescriptionAdd chrome://quota-internals/ resources
(split from issue 7038034)
BUG=84397
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90447
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90623
Patch Set 1 #Patch Set 2 : add quota_internals_ui.{h,cc} #
Total comments: 6
Patch Set 3 : '' #
Total comments: 14
Patch Set 4 : cleanup #Patch Set 5 : '' #Patch Set 6 : Add descriptions #
Total comments: 14
Patch Set 7 : '' #
Total comments: 19
Patch Set 8 : Add comment, merge test.html to index.html. #
Total comments: 2
Patch Set 9 : remove test mode and mocks #
Total comments: 28
Patch Set 10 : '' #
Total comments: 15
Patch Set 11 : '' #Patch Set 12 : '' #Patch Set 13 : '' #Messages
Total messages: 31 (0 generated)
Hi. I'm trying to add a new WebUI page, chrome://quota-internals/, to dump internal data of QuotaManager. Could you review them?
http://codereview.chromium.org/7053009/diff/4001/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7053009/diff/4001/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:52: return "text/html"; indent? http://codereview.chromium.org/7053009/diff/4001/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.h (right): http://codereview.chromium.org/7053009/diff/4001/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.h:19: ~QuotaInternalsUI() {} virtual? http://codereview.chromium.org/7053009/diff/4001/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.h:36: ~QuotaInternalsHTMLSource() {} virtual?
http://codereview.chromium.org/7053009/diff/4001/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7053009/diff/4001/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:52: return "text/html"; On 2011/05/25 08:36:54, kinuko wrote: > indent? Done. http://codereview.chromium.org/7053009/diff/4001/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.h (right): http://codereview.chromium.org/7053009/diff/4001/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.h:19: ~QuotaInternalsUI() {} On 2011/05/25 08:36:54, kinuko wrote: > virtual? Done. http://codereview.chromium.org/7053009/diff/4001/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.h:36: ~QuotaInternalsHTMLSource() {} On 2011/05/25 08:36:54, kinuko wrote: > virtual? Done.
Arv, estade, michael, can any of you review the resource files under chrome/browser/resources/?
please try to match the coding style guidelines before sending code for review http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... File chrome/browser/resources/quota_internals/quota-internals.css (right): http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... chrome/browser/resources/quota_internals/quota-internals.css:2: body { what http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... chrome/browser/resources/quota_internals/quota-internals.css:5: .sort{text-decoration: underline;} not valid style http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... File chrome/browser/resources/quota_internals/quota-internals.js (right): http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... chrome/browser/resources/quota_internals/quota-internals.js:1: license header http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... chrome/browser/resources/quota_internals/quota-internals.js:2: document.addEventListener("DOMContentLoaded",function(){ please don't inline this function. Follow the cr.define namespacing style http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... chrome/browser/resources/quota_internals/quota-internals.js:4: function LOG() { every function in this file needs a comment http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... chrome/browser/resources/quota_internals/quota-internals.js:222: /* commented out code?
Can you show us a screen shot of what this looks like? Somebody more familiar (Evan?) with how we do .html .js and .css in the webui really needs to look those parts. I'm happy to help with the .cc and .h stuff. http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:16: extra blank lines http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:23: // AddMessageHandler(handler->Attach(this)); maybe just leave a TODO without the comment out code http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:41: std::string full_html(html.data(), html.size()); nit: maybe avoid the string copy, can html_bytes be loaded from html w/o the intermediary copy?
Sorry. I'm trying to clean up my code, mainly .js. http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... File chrome/browser/resources/quota_internals/quota-internals.js (right): http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... chrome/browser/resources/quota_internals/quota-internals.js:1: On 2011/05/26 16:13:13, Evan Stade wrote: > license header Done. http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... chrome/browser/resources/quota_internals/quota-internals.js:222: /* On 2011/05/26 16:13:13, Evan Stade wrote: > commented out code? Done. http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:16: On 2011/05/26 20:28:31, michaeln wrote: > extra blank lines Done. http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:23: // AddMessageHandler(handler->Attach(this)); On 2011/05/26 20:28:31, michaeln wrote: > maybe just leave a TODO without the comment out code Done. http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... chrome/browser/ui/webui/quota_internals_ui.cc:41: std::string full_html(html.data(), html.size()); On 2011/05/26 20:28:31, michaeln wrote: > nit: maybe avoid the string copy, can html_bytes be loaded from html w/o the > intermediary copy? Done.
Now, chrome://quota-internals/ looks like the screen shot attached. It just dumped QuotaManager data. 2011年5月27日23:53 <tzik@chromium.org>: > Sorry. I'm trying to clean up my code, mainly .js. > > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... > File chrome/browser/resources/quota_internals/quota-internals.js > (right): > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... > chrome/browser/resources/quota_internals/quota-internals.js:1: > On 2011/05/26 16:13:13, Evan Stade wrote: >> >> license header > > Done. > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... > chrome/browser/resources/quota_internals/quota-internals.js:222: /* > On 2011/05/26 16:13:13, Evan Stade wrote: >> >> commented out code? > > Done. > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... > File chrome/browser/ui/webui/quota_internals_ui.cc (right): > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... > chrome/browser/ui/webui/quota_internals_ui.cc:16: > On 2011/05/26 20:28:31, michaeln wrote: >> >> extra blank lines > > Done. > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... > chrome/browser/ui/webui/quota_internals_ui.cc:23: // > AddMessageHandler(handler->Attach(this)); > On 2011/05/26 20:28:31, michaeln wrote: >> >> maybe just leave a TODO without the comment out code > > Done. > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... > chrome/browser/ui/webui/quota_internals_ui.cc:41: std::string > full_html(html.data(), html.size()); > On 2011/05/26 20:28:31, michaeln wrote: >> >> nit: maybe avoid the string copy, can html_bytes be loaded from html > > w/o the >> >> intermediary copy? > > Done. > > http://codereview.chromium.org/7053009/ >
The .cc and .h file changes lg > Now, chrome://quota-internals/ looks like the screen shot attached. > It just dumped QuotaManager data. how do we look at the screen shot?
On 2011/05/27 17:22:40, michaeln wrote: > The .cc and .h file changes lg > > > Now, chrome://quota-internals/ looks like the screen shot attached. > > It just dumped QuotaManager data. > > how do we look at the screen shot? It didn't appear on the review site but was attached to the previous email (I found it in my gmail view).
Evan, could you take another look at the updated code? We really want to have this change in M13 if possible. On 2011/05/27 14:59:40, tzik wrote: > Now, chrome://quota-internals/ looks like the screen shot attached. > It just dumped QuotaManager data. > > 2011年5月27日23:53 <tzik@chromium.org>: > > Sorry. I'm trying to clean up my code, mainly .js. > > > > > > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... > > File chrome/browser/resources/quota_internals/quota-internals.js > > (right): > > > > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... > > chrome/browser/resources/quota_internals/quota-internals.js:1: > > On 2011/05/26 16:13:13, Evan Stade wrote: > >> > >> license header > > > > Done. > > > > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/resources/quo... > > chrome/browser/resources/quota_internals/quota-internals.js:222: /* > > On 2011/05/26 16:13:13, Evan Stade wrote: > >> > >> commented out code? > > > > Done. > > > > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... > > File chrome/browser/ui/webui/quota_internals_ui.cc (right): > > > > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... > > chrome/browser/ui/webui/quota_internals_ui.cc:16: > > On 2011/05/26 20:28:31, michaeln wrote: > >> > >> extra blank lines > > > > Done. > > > > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... > > chrome/browser/ui/webui/quota_internals_ui.cc:23: // > > AddMessageHandler(handler->Attach(this)); > > On 2011/05/26 20:28:31, michaeln wrote: > >> > >> maybe just leave a TODO without the comment out code > > > > Done. > > > > > http://codereview.chromium.org/7053009/diff/3002/chrome/browser/ui/webui/quot... > > chrome/browser/ui/webui/quota_internals_ui.cc:41: std::string > > full_html(html.data(), html.size()); > > On 2011/05/26 20:28:31, michaeln wrote: > >> > >> nit: maybe avoid the string copy, can html_bytes be loaded from html > > > > w/o the > >> > >> intermediary copy? > > > > Done. > > > > http://codereview.chromium.org/7053009/ > >
The layout of the page doesn't have much context, I don't know what I'm looking at except by inspecting the source (global data, host data, origin data), so that needs some design work (was there a UI designer associated with this feature?) I don't know if this can make the m13 timeline given how far the code is from feature-complete (and it appears the strings will be past the string due-date as well). http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/index.html (right): http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/index.html:16: <div>Available Disk Space: <span id="diskspace-entry">N/A</span></div> Is there a reason not to localize these strings? Even if there is a good reason, you should put the strings into a .grd file somewhere (probably generated_resources.grd) with translateable=false because you seem to have several strings repeated here. http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/mock_chrome_message.js (right): http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/mock_chrome_message.js:17: * @type {{ needs textual explanation http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/mock_chrome_message.js:18: * type: {!string}, these need textual explanations as well http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/mock_chrome_message.js:26: unlimited_usage: '100', this one is not documented http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/mock_chrome_message.js:44: { http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... curlies do not get their own line http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/quota-internals.js (right): http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:18: triggar: function() { trigger? http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:22: // Chrome Message Handler why are you doing this instead of making the webui handler call js functions directly via CallJavascriptFunction() http://codereview.chromium.org/7053009/diff/19001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7053009/diff/19001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_ui.cc:31: bool is_incognito, indentation is off
Thanks for reviewing. On Wed, Jun 1, 2011 at 9:48 AM, <estade@chromium.org> wrote: > The layout of the page doesn't have much context, I don't know what I'm > looking > at except by inspecting the source (global data, host data, origin data), > so > that needs some design work (was there a UI designer associated with this > feature?) > Let me give you some background context: The page is to show the internal status of the quota, which is a new feature to be added in M13 ( crbug.com/61676), and is mainly for internal development/debugging. We don't expect regular users see/interpret this page in normal cases and therefore no UI designer is associated with this page. One possible usage scenario where external users/developers see this page is: when a bug is filed for the feature we'll ask the reporter to copy&paste the page content. We do have been talking wih UI leads/designers for the user-facing UI of the feature though. For the same reason (this is not for regular users) I do not think it's mandatory to localize the strings. I don't know if this can make the m13 timeline given how far the code is > from > feature-complete (and it appears the strings will be past the string > due-date as > well). > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > File chrome/browser/resources/quota_internals/index.html (right): > > > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/index.html:16: <div>Available > Disk Space: <span id="diskspace-entry">N/A</span></div> > Is there a reason not to localize these strings? Even if there is a good > reason, you should put the strings into a .grd file somewhere (probably > generated_resources.grd) with translateable=false because you seem to > have several strings repeated here. > > > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > File chrome/browser/resources/quota_internals/mock_chrome_message.js > (right): > > > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/mock_chrome_message.js:17: * > @type {{ > needs textual explanation > > > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/mock_chrome_message.js:18: * > type: {!string}, > these need textual explanations as well > > > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/mock_chrome_message.js:26: > unlimited_usage: '100', > this one is not documented > > > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/mock_chrome_message.js:44: { > > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... > > curlies do not get their own line > > > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > > File chrome/browser/resources/quota_internals/quota-internals.js > (right): > > > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/quota-internals.js:18: triggar: > function() { > trigger? > > > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/quota-internals.js:22: // > Chrome Message Handler > why are you doing this instead of making the webui handler call js > functions directly via CallJavascriptFunction() > > > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/ui/webui/quo... > > File chrome/browser/ui/webui/quota_internals_ui.cc (right): > > > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/ui/webui/quo... > chrome/browser/ui/webui/quota_internals_ui.cc:31: bool is_incognito, > indentation is off > > > http://codereview.chromium.org/7053009/ >
thanks for the background. I still think it needs a bit more work UI-design-wise if developers are meant to use it productively. If you look at net-internals, while it is unattractive and clearly not meant to be user-facing, there's still enough context built into the page (via headers, tab titles, and so forth) that you don't have to read code to figure out what the numbers mean. Analogously, users never read code or comments but there is still a standard of presentability for them (codified in the style guidelines). As far as localization, a short note with your explanation in the code would suffice. As far as the review goes, I still think the code needs some work. The fact that the feature is not user-facing, and the spectre of a looming deadline, are not in my mind reason enough to commit. If this is really a super-critical feature for m13, it can be merged to the branch once it's brought into better alignment with our other code. (Note that many webui pages are quite old, and have rather regrettable style. That is likewise not a reason to go on committing unpolished code.) If it's not super-critical, this is why we have a 6 week release cycle. I would urge anyone who wants to get their feature into a certain milestone to upload their code for review more than 3 workdays before the deadline. -- Evan Stade On Tue, May 31, 2011 at 6:34 PM, Kinuko Yasuda <kinuko@chromium.org> wrote: > Thanks for reviewing. > On Wed, Jun 1, 2011 at 9:48 AM, <estade@chromium.org> wrote: >> >> The layout of the page doesn't have much context, I don't know what I'm >> looking >> at except by inspecting the source (global data, host data, origin data), >> so >> that needs some design work (was there a UI designer associated with this >> feature?) > > Let me give you some background context: The page is to show the internal > status of the quota, which is a new feature to be added in M13 > (crbug.com/61676), and is mainly for internal development/debugging. >  We don't expect regular users see/interpret this page in normal cases and > therefore no UI designer is associated with this page.   One possible usage > scenario where external users/developers see this page is: when a bug is > filed for the feature we'll ask the reporter to copy&paste the page content. > We do have been talking wih UI leads/designers for the user-facing UI of the > feature though. > For the same reason (this is not for regular users) I do not think it's > mandatory to localize the strings. >> >> I don't know if this can make the m13 timeline given how far the code is >> from >> feature-complete (and it appears the strings will be past the string >> due-date as >> well). > >> >> http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... >> File chrome/browser/resources/quota_internals/index.html (right): >> >> >> http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... >> chrome/browser/resources/quota_internals/index.html:16: <div>Available >> Disk Space: <span id="diskspace-entry">N/A</span></div> >> Is there a reason not to localize these strings? Even if there is a good >> reason, you should put the strings into a .grd file somewhere (probably >> generated_resources.grd) with translateable=false because you seem to >> have several strings repeated here. >> >> >> http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... >> File chrome/browser/resources/quota_internals/mock_chrome_message.js >> (right): >> >> >> http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... >> chrome/browser/resources/quota_internals/mock_chrome_message.js:17: * >> @type {{ >> needs textual explanation >> >> >> http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... >> chrome/browser/resources/quota_internals/mock_chrome_message.js:18: * >>   type: {!string}, >> these need textual explanations as well >> >> >> http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... >> chrome/browser/resources/quota_internals/mock_chrome_message.js:26: >> unlimited_usage: '100', >> this one is not documented >> >> >> http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... >> chrome/browser/resources/quota_internals/mock_chrome_message.js:44: { >> >> http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... >> >> curlies do not get their own line >> >> >> http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... >> File chrome/browser/resources/quota_internals/quota-internals.js >> (right): >> >> >> http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... >> chrome/browser/resources/quota_internals/quota-internals.js:18: triggar: >> function() { >> trigger? >> >> >> http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... >> chrome/browser/resources/quota_internals/quota-internals.js:22: // >> Chrome Message Handler >> why are you doing this instead of making the webui handler call js >> functions directly via CallJavascriptFunction() >> >> >> http://codereview.chromium.org/7053009/diff/19001/chrome/browser/ui/webui/quo... >> File chrome/browser/ui/webui/quota_internals_ui.cc (right): >> >> >> http://codereview.chromium.org/7053009/diff/19001/chrome/browser/ui/webui/quo... >> chrome/browser/ui/webui/quota_internals_ui.cc:31: bool is_incognito, >> indentation is off >> >> http://codereview.chromium.org/7053009/ > >
On Wed, Jun 1, 2011 at 11:28 AM, Evan Stade <estade@chromium.org> wrote: > thanks for the background. > > I still think it needs a bit more work UI-design-wise if developers > are meant to use it productively. If you look at net-internals, while > it is unattractive and clearly not meant to be user-facing, there's > still enough context built into the page (via headers, tab titles, and > so forth) that you don't have to read code to figure out what the > numbers mean. Analogously, users never read code or comments but there > is still a standard of presentability for them (codified in the style > guidelines). > > As far as localization, a short note with your explanation in the code > would suffice. > > As far as the review goes, I still think the code needs some work. The > fact that the feature is not user-facing, and the spectre of a looming > deadline, are not in my mind reason enough to commit. If this is > really a super-critical feature for m13, it can be merged to the > branch once it's brought into better alignment with our other code. > (Note that many webui pages are quite old, and have rather regrettable > style. That is likewise not a reason to go on committing unpolished > code.) If it's not super-critical, this is why we have a 6 week > release cycle. I would urge anyone who wants to get their feature into > a certain milestone to upload their code for review more than 3 > workdays before the deadline. As for the timeline I totally agree with you. Pushing this patch at this timing is probably too much haste... we'll continue to work on this change (and will ask you some more reviews) and will decide how we should do when we (including you) think it's good to go. Thanks, > > -- Evan Stade > > > > On Tue, May 31, 2011 at 6:34 PM, Kinuko Yasuda <kinuko@chromium.org> > wrote: > > Thanks for reviewing. > > On Wed, Jun 1, 2011 at 9:48 AM, <estade@chromium.org> wrote: > >> > >> The layout of the page doesn't have much context, I don't know what I'm > >> looking > >> at except by inspecting the source (global data, host data, origin > data), > >> so > >> that needs some design work (was there a UI designer associated with > this > >> feature?) > > > > Let me give you some background context: The page is to show the internal > > status of the quota, which is a new feature to be added in M13 > > (crbug.com/61676), and is mainly for internal development/debugging. > > We don't expect regular users see/interpret this page in normal cases > and > > therefore no UI designer is associated with this page. One possible > usage > > scenario where external users/developers see this page is: when a bug is > > filed for the feature we'll ask the reporter to copy&paste the page > content. > > We do have been talking wih UI leads/designers for the user-facing UI of > the > > feature though. > > For the same reason (this is not for regular users) I do not think it's > > mandatory to localize the strings. > >> > >> I don't know if this can make the m13 timeline given how far the code is > >> from > >> feature-complete (and it appears the strings will be past the string > >> due-date as > >> well). > > > >> > >> > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > >> File chrome/browser/resources/quota_internals/index.html (right): > >> > >> > >> > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > >> chrome/browser/resources/quota_internals/index.html:16: <div>Available > >> Disk Space: <span id="diskspace-entry">N/A</span></div> > >> Is there a reason not to localize these strings? Even if there is a good > >> reason, you should put the strings into a .grd file somewhere (probably > >> generated_resources.grd) with translateable=false because you seem to > >> have several strings repeated here. > >> > >> > >> > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > >> File chrome/browser/resources/quota_internals/mock_chrome_message.js > >> (right): > >> > >> > >> > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > >> chrome/browser/resources/quota_internals/mock_chrome_message.js:17: * > >> @type {{ > >> needs textual explanation > >> > >> > >> > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > >> chrome/browser/resources/quota_internals/mock_chrome_message.js:18: * > >> type: {!string}, > >> these need textual explanations as well > >> > >> > >> > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > >> chrome/browser/resources/quota_internals/mock_chrome_message.js:26: > >> unlimited_usage: '100', > >> this one is not documented > >> > >> > >> > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > >> chrome/browser/resources/quota_internals/mock_chrome_message.js:44: { > >> > >> > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... > >> > >> curlies do not get their own line > >> > >> > >> > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > >> File chrome/browser/resources/quota_internals/quota-internals.js > >> (right): > >> > >> > >> > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > >> chrome/browser/resources/quota_internals/quota-internals.js:18: triggar: > >> function() { > >> trigger? > >> > >> > >> > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... > >> chrome/browser/resources/quota_internals/quota-internals.js:22: // > >> Chrome Message Handler > >> why are you doing this instead of making the webui handler call js > >> functions directly via CallJavascriptFunction() > >> > >> > >> > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/ui/webui/quo... > >> File chrome/browser/ui/webui/quota_internals_ui.cc (right): > >> > >> > >> > http://codereview.chromium.org/7053009/diff/19001/chrome/browser/ui/webui/quo... > >> chrome/browser/ui/webui/quota_internals_ui.cc:31: bool is_incognito, > >> indentation is off > >> > >> http://codereview.chromium.org/7053009/ > > > > >
Thanks for your reviewing. I try to refine UI, refering sync-internals. Some screenshots are available on: http://code.google.com/p/chromium/issues/detail?id=84397#c6 Could you give some advice for me? http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/index.html (right): http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/index.html:16: <div>Available Disk Space: <span id="diskspace-entry">N/A</span></div> On 2011/06/01 00:48:25, Evan Stade wrote: > Is there a reason not to localize these strings? Even if there is a good reason, > you should put the strings into a .grd file somewhere (probably > generated_resources.grd) with translateable=false because you seem to have > several strings repeated here. Refering other chrome://***-internals/, we had localized only a few strings. (e.g. text direction, fontfamily, fontsize.) How should we decide to do or not? http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/mock_chrome_message.js (right): http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/mock_chrome_message.js:26: unlimited_usage: '100', On 2011/06/01 00:48:25, Evan Stade wrote: > this one is not documented Done. http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/mock_chrome_message.js:44: { On 2011/06/01 00:48:25, Evan Stade wrote: > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... > > curlies do not get their own line Done. http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/quota-internals.js (right): http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:18: triggar: function() { On 2011/06/01 00:48:25, Evan Stade wrote: > trigger? I couldn't make good name for it. I just renamed it requestData. http://codereview.chromium.org/7053009/diff/19001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:22: // Chrome Message Handler On 2011/06/01 00:48:25, Evan Stade wrote: > why are you doing this instead of making the webui handler call js functions > directly via CallJavascriptFunction() I think, this style makes the page more extendable. Each tab can get data and be notified independently with Events. http://codereview.chromium.org/7053009/diff/19001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7053009/diff/19001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_ui.cc:31: bool is_incognito, On 2011/06/01 00:48:25, Evan Stade wrote: > indentation is off Done.
overall, I request more high-level comments. Can I ask why you created the testing mode, what is insufficient about loading the actual page in chrome? http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/event_handler.js (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:41: var normalizer_ = { comments for all of these http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:413: function dump() { when is this used? http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/index.html (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/index.html:29: <tabbox id="tabboxes"> what is this id used for? http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/mock_chrome_message.js (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/mock_chrome_message.js:8: if (!window.chrome || !chrome.send) { file comment! http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/mock_strings.js (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/mock_strings.js:5: cr.define('cr.quota', function() { again, please add a comment explaining this file http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/quota-internals.js (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:18: requestData: function() { can you define these function first and then just return them by name: return { foo: foo, bar: bar, ... } http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:23: messageHandler_: function(message, detail) { every function needs a comment. Every parameter needs a comment. http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/test.html (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/test.html:1: <!DOCTYPE html> it seems bad to have 2 almost identical files, although I'm not sure how to fix this. http://codereview.chromium.org/7053009/diff/28001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_ui.h (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_ui.h:17: explicit QuotaInternalsUI(TabContents* contents); define virtual destructor http://codereview.chromium.org/7053009/diff/28001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_ui.h:28: QuotaInternalsHTMLSource(); define virtual destructor
Thank you for reviewing again. I added some comments, merged test.html into index.html and removed unused code. (these modifications are mainly on .js and .html files). On 2011/06/04 02:43:17, Evan Stade wrote: > overall, I request more high-level comments. > > Can I ask why you created the testing mode, what is insufficient about loading > the actual page in chrome? It's for faster reloading. After modifing resources, we have to rebuild chrome to refresh chrome://quota-internals/. It takes about 30 seconds on my machine, while plain html page can be reloaded instantly. http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/event_handler.js (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:41: var normalizer_ = { On 2011/06/04 02:43:17, Evan Stade wrote: > comments for all of these Done. http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:413: function dump() { On 2011/06/04 02:43:17, Evan Stade wrote: > when is this used? That is used below, as event handler of dump-button "click". http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/index.html (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/index.html:29: <tabbox id="tabboxes"> On 2011/06/04 02:43:17, Evan Stade wrote: > what is this id used for? Done. It is no longer used. http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/mock_chrome_message.js (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/mock_chrome_message.js:8: if (!window.chrome || !chrome.send) { On 2011/06/04 02:43:17, Evan Stade wrote: > file comment! Done. http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/mock_strings.js (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/mock_strings.js:5: cr.define('cr.quota', function() { On 2011/06/04 02:43:17, Evan Stade wrote: > again, please add a comment explaining this file Done. http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/quota-internals.js (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:18: requestData: function() { On 2011/06/04 02:43:17, Evan Stade wrote: > can you define these function first and then just return them by name: > > return { > foo: foo, > bar: bar, > ... > } Done. http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/test.html (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/test.html:1: <!DOCTYPE html> On 2011/06/04 02:43:17, Evan Stade wrote: > it seems bad to have 2 almost identical files, although I'm not sure how to fix > this. Done. http://codereview.chromium.org/7053009/diff/28001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_ui.h (right): http://codereview.chromium.org/7053009/diff/28001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_ui.h:17: explicit QuotaInternalsUI(TabContents* contents); On 2011/06/04 02:43:17, Evan Stade wrote: > define virtual destructor Done. http://codereview.chromium.org/7053009/diff/28001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_ui.h:28: QuotaInternalsHTMLSource(); On 2011/06/04 02:43:17, Evan Stade wrote: > define virtual destructor Done.
if you are on Linux, try just remaking packed_resources and chrome_extra_resources. Here's a run after I touch content_settings.html: $ time make packed_resources chrome_extra_resources BUILDTYPE=Release make: Nothing to be done for `packed_resources'. ACTION Generating resources from browser/resources/options_resources.grd out/Release/obj/gen/chrome/grit/options_resources.h real 0m8.063s user 0m7.570s sys 0m0.410s So that's 8 seconds instead of 30. Still not instant, but I'm not sure saving 8 seconds is worth all the overhead of supporting a standalone version of this page. For example, if someone adds a new message they'd have to update mock_chrome_message.js. In the short term this would likely be you, but if anyone else started to work on this page, since nothing enforces updating the test code, it would get broken. It just seems rather fragile and we it also gets more and more complex as the page gets more complex. In summary, I assume you find it useful, so you can keep using it, but I think this test code belongs on your local machine, not in the chrome repo. On 2011/06/06 10:57:44, tzik wrote: > Thank you for reviewing again. > I added some comments, merged test.html into index.html and removed unused code. > (these modifications are mainly on .js and .html files). > > > On 2011/06/04 02:43:17, Evan Stade wrote: > > overall, I request more high-level comments. > > > > Can I ask why you created the testing mode, what is insufficient about loading > > the actual page in chrome? > > > It's for faster reloading. > After modifing resources, we have to rebuild chrome to refresh > chrome://quota-internals/. > It takes about 30 seconds on my machine, while plain html page can be reloaded > instantly. > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > File chrome/browser/resources/quota_internals/event_handler.js (right): > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/event_handler.js:41: var normalizer_ = > { > On 2011/06/04 02:43:17, Evan Stade wrote: > > comments for all of these > > Done. > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/event_handler.js:413: function dump() { > On 2011/06/04 02:43:17, Evan Stade wrote: > > when is this used? > > That is used below, as event handler of dump-button "click". > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > File chrome/browser/resources/quota_internals/index.html (right): > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/index.html:29: <tabbox id="tabboxes"> > On 2011/06/04 02:43:17, Evan Stade wrote: > > what is this id used for? > > Done. It is no longer used. > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > File chrome/browser/resources/quota_internals/mock_chrome_message.js (right): > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/mock_chrome_message.js:8: if > (!window.chrome || !chrome.send) { > On 2011/06/04 02:43:17, Evan Stade wrote: > > file comment! > > Done. > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > File chrome/browser/resources/quota_internals/mock_strings.js (right): > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/mock_strings.js:5: > cr.define('cr.quota', function() { > On 2011/06/04 02:43:17, Evan Stade wrote: > > again, please add a comment explaining this file > > Done. > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > File chrome/browser/resources/quota_internals/quota-internals.js (right): > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/quota-internals.js:18: requestData: > function() { > On 2011/06/04 02:43:17, Evan Stade wrote: > > can you define these function first and then just return them by name: > > > > return { > > foo: foo, > > bar: bar, > > ... > > } > > Done. > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > File chrome/browser/resources/quota_internals/test.html (right): > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/resources/qu... > chrome/browser/resources/quota_internals/test.html:1: <!DOCTYPE html> > On 2011/06/04 02:43:17, Evan Stade wrote: > > it seems bad to have 2 almost identical files, although I'm not sure how to > fix > > this. > > Done. > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/ui/webui/quo... > File chrome/browser/ui/webui/quota_internals_ui.h (right): > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/ui/webui/quo... > chrome/browser/ui/webui/quota_internals_ui.h:17: explicit > QuotaInternalsUI(TabContents* contents); > On 2011/06/04 02:43:17, Evan Stade wrote: > > define virtual destructor > > Done. > > http://codereview.chromium.org/7053009/diff/28001/chrome/browser/ui/webui/quo... > chrome/browser/ui/webui/quota_internals_ui.h:28: QuotaInternalsHTMLSource(); > On 2011/06/04 02:43:17, Evan Stade wrote: > > define virtual destructor > > Done.
(forgot to publish comments) http://codereview.chromium.org/7053009/diff/34002/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/index.html (right): http://codereview.chromium.org/7053009/diff/34002/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/index.html:28: <!-- This Block is disabled when the page is built into binary. --> I'm not sure how I feel about committing debug code into the repo, this is basically equivalent to C++ #if 0, which would not be allowed.
> if you are on Linux, try just remaking packe d_resources and > chrome_extra_resources. Here's a run after I touch content_settings.html: Thank you. Remaking packed_extra_resources was sufficient for me. I agree with that there is no automated test for this page and it is hard to keep consistency. So I removed it from this patch and I'll use it only locally. http://codereview.chromium.org/7053009/diff/34002/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/index.html (right): http://codereview.chromium.org/7053009/diff/34002/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/index.html:28: <!-- This Block is disabled when the page is built into binary. --> On 2011/06/07 03:40:36, Evan Stade wrote: > I'm not sure how I feel about committing debug code into the repo, this is > basically equivalent to C++ #if 0, which would not be allowed. I don't think so. #if 0 block will not work anywhere, but this block works in some scene. I think it is equivalent to NDEBUG block.
http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/event_handler.js (right): http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:31: var overwrite_ = function(src, dest) { notate privacy with @private on the line after @return http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:46: var normalizer_ = { comment for this class of functions http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:52: avail_: function(v) { prefer to avoid abbreviating symbol names. Perhaps available for the function and value for the parameter http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:62: bool: function(v) { I don't see this function used anywhere (?) http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:69: * Returns |v| itself unless |v| is not undefined, I think 'unless' should be 'if' http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:140: var available_space = undefined; camel case (for all variables, except const, which is YELL_CASE.) http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/quota-internals.js (right): http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:41: * @param {Object} detais Message specific additional data. s/detais/detail http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:43: function messageHandler_(message, detail) { remove final underscore http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:46: case 'AvailableSpaceUpdated': everything inside switch should be indented 2 spaces more http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:62: console.log('Unknown Message'); probably worthy of a console.error http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:65: var event = cr.doc.createEvent('CustomEvent'); move inside if statement? http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals_resources.grd (right): http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals_resources.grd:2: <!-- This comment is only here because changes to resources are not picked up Do you know if this comment is actually needed? I think it may be obsolete (if my guess is correct, it was due to an old incredibuild bug) http://codereview.chromium.org/7053009/diff/39001/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_data_source.h (right): http://codereview.chromium.org/7053009/diff/39001/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_data_source.h:21: // Adds a name and its equivaled localized string to our dictionary. equivalent? http://codereview.chromium.org/7053009/diff/39001/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_data_source.h:26: std::string MakeLocalizedStringsAsJSON(); where is this defined? http://codereview.chromium.org/7053009/diff/39001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7053009/diff/39001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_ui.cc:49: std::string js( this is not a very standard way to add strings, I think you are doing it because of your debug code? It's going to be hard for the next person to know this, or know whether they are breaking your debug code. Also, it's a lot more verbose than the normal way to add localized strings. I'm not convinced we want to localize the strings (I was genuinely curious when I asked why we weren't translating them). I guess it depends on your envisioned use for this page. If it's only for developers, it might be better to leave it in English so that if you ask a user to copy the page's contents, you get something you can read, rather than something the user can read. I think your mock strings approach was nice because it lets you store re-used strings in one place but it doesn't need to involve c++ since we don't want translations (unless you decide we do want translations).
http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/event_handler.js (right): http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:31: var overwrite_ = function(src, dest) { On 2011/06/07 16:54:44, Evan Stade wrote: > notate privacy with @private on the line after @return Done. http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:52: avail_: function(v) { On 2011/06/07 16:54:44, Evan Stade wrote: > prefer to avoid abbreviating symbol names. Perhaps available for the function > and value for the parameter Done. http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:62: bool: function(v) { On 2011/06/07 16:54:44, Evan Stade wrote: > I don't see this function used anywhere (?) Done. http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:69: * Returns |v| itself unless |v| is not undefined, On 2011/06/07 16:54:44, Evan Stade wrote: > I think 'unless' should be 'if' Done. http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:140: var available_space = undefined; On 2011/06/07 16:54:44, Evan Stade wrote: > camel case (for all variables, except const, which is YELL_CASE.) Done. http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/quota-internals.js (right): http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:41: * @param {Object} detais Message specific additional data. On 2011/06/07 16:54:44, Evan Stade wrote: > s/detais/detail Done. http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:43: function messageHandler_(message, detail) { On 2011/06/07 16:54:44, Evan Stade wrote: > remove final underscore Done. http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:46: case 'AvailableSpaceUpdated': On 2011/06/07 16:54:44, Evan Stade wrote: > everything inside switch should be indented 2 spaces more Done. http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:62: console.log('Unknown Message'); On 2011/06/07 16:54:44, Evan Stade wrote: > probably worthy of a console.error Done. http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.js:65: var event = cr.doc.createEvent('CustomEvent'); On 2011/06/07 16:54:44, Evan Stade wrote: > move inside if statement? Done. http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals_resources.grd (right): http://codereview.chromium.org/7053009/diff/39001/chrome/browser/resources/qu... chrome/browser/resources/quota_internals_resources.grd:2: <!-- This comment is only here because changes to resources are not picked up On 2011/06/07 16:54:44, Evan Stade wrote: > Do you know if this comment is actually needed? I think it may be obsolete (if > my guess is correct, it was due to an old incredibuild bug) Done. http://codereview.chromium.org/7053009/diff/39001/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_data_source.h (right): http://codereview.chromium.org/7053009/diff/39001/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_data_source.h:26: std::string MakeLocalizedStringsAsJSON(); On 2011/06/07 16:54:44, Evan Stade wrote: > where is this defined? It's a contamination from local change. Thanks. http://codereview.chromium.org/7053009/diff/39001/chrome/browser/ui/webui/quo... File chrome/browser/ui/webui/quota_internals_ui.cc (right): http://codereview.chromium.org/7053009/diff/39001/chrome/browser/ui/webui/quo... chrome/browser/ui/webui/quota_internals_ui.cc:49: std::string js( On 2011/06/07 16:54:44, Evan Stade wrote: > this is not a very standard way to add strings, I think you are doing it because > of your debug code? It's going to be hard for the next person to know this, or > know whether they are breaking your debug code. Also, it's a lot more verbose > than the normal way to add localized strings. > > I'm not convinced we want to localize the strings (I was genuinely curious when > I asked why we weren't translating them). I guess it depends on your envisioned > use for this page. If it's only for developers, it might be better to leave it > in English so that if you ask a user to copy the page's contents, you get > something you can read, rather than something the user can read. I think your > mock strings approach was nice because it lets you store re-used strings in one > place but it doesn't need to involve c++ since we don't want translations > (unless you decide we do want translations). I see, I was mistaken as if this page must be internationalized. As just a aside. I think, localization is page specific matter in general. So localization strings shold be in their own namespace, should not be in global namespace. Standard way (maybe, jstemplate_builder::AppendJsonJS() through ChromeWebUIDataSource::SendLocalizedStringsAsJSON()) will lead us to place localized strings as global fixed named object. This does not look good for me. It is why I wanted to avoid that. On other hand, also it doesn't look good for me that, WebUI page depends on detail of the resources, such as dependency of each resource. So, I want to avoid using AddLocalizedString in C++ source, even if we do translate it. Anyway, if we decide not to internationalize the page at all, we can throw away the issue. And I'll keep it developer's page and leave it in English.
LGTM with nits addressed http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/event_handler.js (right): http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:33: function overwrite_(source, destination) { how about calling this copyAttributes_? http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:120: * UNIX epock time (0:00, Jan 1, 1970, UTC). epoch http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:183: * Initialize and return |tree_view_object|. these comments are out of sync with the variable names (also, elsewhere in the file) http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:331: var data_array = event.detail; dateArray http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:359: * and some additional field can be included. s/field/fields http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:373: var data_array = event.detail; dataArray http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:454: function dumpTreeToObj(opt_treeitem) { camel case
http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/event_handler.js (right): http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:33: function overwrite_(source, destination) { On 2011/06/14 01:37:00, Evan Stade wrote: > how about calling this copyAttributes_? Done. http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:120: * UNIX epock time (0:00, Jan 1, 1970, UTC). On 2011/06/14 01:37:00, Evan Stade wrote: > epoch Done. http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:183: * Initialize and return |tree_view_object|. On 2011/06/14 01:37:00, Evan Stade wrote: > these comments are out of sync with the variable names (also, elsewhere in the > file) Done. http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:331: var data_array = event.detail; On 2011/06/14 01:37:00, Evan Stade wrote: > dateArray Done. http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:359: * and some additional field can be included. On 2011/06/14 01:37:00, Evan Stade wrote: > s/field/fields Done. http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:373: var data_array = event.detail; On 2011/06/14 01:37:00, Evan Stade wrote: > dataArray Done. http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/event_handler.js:454: function dumpTreeToObj(opt_treeitem) { On 2011/06/14 01:37:00, Evan Stade wrote: > camel case I use opt_ as a tag for optional variable, written in below. http://www.corp.google.com/eng/doc/javascriptguide.xml?showone=Naming#Naming http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... File chrome/browser/resources/quota_internals/quota-internals.css (right): http://codereview.chromium.org/7053009/diff/46007/chrome/browser/resources/qu... chrome/browser/resources/quota_internals/quota-internals.css:24: background-image: url("../shared/images/icon_file.png"); shared/images/icon_file.png has gone.
Is this still LG after this change?
1. why is that icon gone? 2. there should not be hyphens in file names
On 2011/06/20 18:52:36, Evan Stade wrote: > 1. why is that icon gone? The icon was no longer used anywhere else, so it was deleted in this change set. http://codereview.chromium.org/6995116 I'll re-add it. > 2. there should not be hyphens in file names I see. I'll rename it. How about renaming like below? index.html to main.html, quota-internals.css to main.css, quota-internals.js to message_dispatcher.js.
I renamed: index.html to main.html, quota-internals.css to main.css, quota-internals.js to message_dispatcher.js. And re-add chrome/browser/resources/shared/images/icon_file.png. How about it this time?
lgtm
Thanks, I checked the commit checkbox. |
