Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808100:
Code implements UI for downloading and burning Chrome OS images on SSD card a... (Closed)
|
Created:
10 years, 4 months ago by tonibarzic Modified:
9 years, 4 months ago CC:
chromium-reviews, ben+cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionCode implements UI for downloading and burning Chrome OS images on SSD card and USB key. Actual burning is not included in the change list.
BUG=chromium-os:5346
TEST=type in chrome://imagebuner in browser. UI lists all media that image can be burnt to. After selecting burning target by clicking an image right to the target name, image download should start, and download progress should be displayed. After download ends alert should pop up asking user to confirm that he wants to burn image to selected device. Clicking both ok or cancel shouldn't do anything, since actual burning isn't stil included in CL.
Image should be downloaded to chrome_image folder in users Downloads directory.
this folder is deleted during shutdown.
This is only visible in ChromeOS...
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57085
Patch Set 1 #
Total comments: 82
Patch Set 2 : '' #
Total comments: 66
Patch Set 3 : '' #
Total comments: 8
Patch Set 4 : '' #
Total comments: 16
Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 13
Patch Set 11 : '' #Patch Set 12 : '' #
Total comments: 6
Messages
Total messages: 22 (0 generated)
You should really use the HTML5 progress element. It will save you hundreds of lines of code. I also find all the single statement functions a bit confusing. Can you just inline them? http://codereview.chromium.org/2808100/diff/1/7 File chrome/browser/resources/imageburner.html (right): http://codereview.chromium.org/2808100/diff/1/7#newcode1 chrome/browser/resources/imageburner.html:1: <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"> <!DOCTYPE html> http://codereview.chromium.org/2808100/diff/1/7#newcode4 chrome/browser/resources/imageburner.html:4: <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> <met charset="UTF-8"> http://codereview.chromium.org/2808100/diff/1/7#newcode6 chrome/browser/resources/imageburner.html:6: <style type="text/css"> <style> http://codereview.chromium.org/2808100/diff/1/7#newcode7 chrome/browser/resources/imageburner.html:7: div.header { looks like a header to me. Can you use Hx? http://codereview.chromium.org/2808100/diff/1/7#newcode16 chrome/browser/resources/imageburner.html:16: box-sizing: border-box; box-sizing is not yet supported. http://codereview.chromium.org/2808100/diff/1/7#newcode22 chrome/browser/resources/imageburner.html:22: border-bottom-color: #999; It seems like you can simplify this quite a lot border: 1px solid #99l border-top: 0; http://codereview.chromium.org/2808100/diff/1/7#newcode43 chrome/browser/resources/imageburner.html:43: width: 90%; This seems pretty arbitrary http://codereview.chromium.org/2808100/diff/1/7#newcode64 chrome/browser/resources/imageburner.html:64: margin-left: -44px; -webkit-margin-start? http://codereview.chromium.org/2808100/diff/1/7#newcode80 chrome/browser/resources/imageburner.html:80: align: right; align is not a valid CSS property Did you mean text-align? text-align: end http://codereview.chromium.org/2808100/diff/1/7#newcode87 chrome/browser/resources/imageburner.html:87: margin-left: 0; -webkit-margin-start -webkit-margin-end http://codereview.chromium.org/2808100/diff/1/7#newcode92 chrome/browser/resources/imageburner.html:92: -webkit-transition: opacity 0.2s ease-out ; .15s is the standard transition time in chrome http://codereview.chromium.org/2808100/diff/1/7#newcode105 chrome/browser/resources/imageburner.html:105: -webkit-transition: opacity 0.0s ease-out ; transition: none? http://codereview.chromium.org/2808100/diff/1/7#newcode109 chrome/browser/resources/imageburner.html:109: opacity: 1.0; opacity: 1; http://codereview.chromium.org/2808100/diff/1/7#newcode132 chrome/browser/resources/imageburner.html:132: margin-left: -22px; -webkit-margin-start http://codereview.chromium.org/2808100/diff/1/7#newcode158 chrome/browser/resources/imageburner.html:158: -webkit-transition: color 1.0s ease-out ; Isn't 1s is too slow? http://codereview.chromium.org/2808100/diff/1/7#newcode253 chrome/browser/resources/imageburner.html:253: div.progressBarRight { How about using the <progress> element :-) http://codereview.chromium.org/2808100/diff/1/7#newcode295 chrome/browser/resources/imageburner.html:295: height:50%; Missing whitespace after : http://codereview.chromium.org/2808100/diff/1/7#newcode297 chrome/browser/resources/imageburner.html:297: padding-top: 1%; What are trying to achieve here? http://codereview.chromium.org/2808100/diff/1/7#newcode317 chrome/browser/resources/imageburner.html:317: border-top: solid 1px #7289E2; 1px solid #7289e2 for consistency http://codereview.chromium.org/2808100/diff/1/7#newcode337 chrome/browser/resources/imageburner.html:337: var rootsDiv = null; do you really need to assign null here? http://codereview.chromium.org/2808100/diff/1/7#newcode340 chrome/browser/resources/imageburner.html:340: var kListTitle; const LIST_TITLE? http://codereview.chromium.org/2808100/diff/1/7#newcode344 chrome/browser/resources/imageburner.html:344: function PromtUserDownloadFinished() { promptuserDownloadFinished Only class names and enums should be capitalized http://codereview.chromium.org/2808100/diff/1/7#newcode346 chrome/browser/resources/imageburner.html:346: var answer = confirm(localStrings.getString('burnConfirmText1')+ ws around binops http://codereview.chromium.org/2808100/diff/1/7#newcode372 chrome/browser/resources/imageburner.html:372: var main = $('rootsColumn'); tabs? http://codereview.chromium.org/2808100/diff/1/7#newcode379 chrome/browser/resources/imageburner.html:379: if(currentlySelectedItem) if (...) http://codereview.chromium.org/2808100/diff/1/7#newcode387 chrome/browser/resources/imageburner.html:387: return function() { fix indentation http://codereview.chromium.org/2808100/diff/1/7#newcode522 chrome/browser/resources/imageburner.html:522: link.onclick = getFunctionForItem(path, element.id); Just inline the function here. http://codereview.chromium.org/2808100/diff/1/7#newcode526 chrome/browser/resources/imageburner.html:526: icon.className = 'icon iconfolder'; ws http://codereview.chromium.org/2808100/diff/1/7#newcode556 chrome/browser/resources/imageburner.html:556: while (list.childNodes.length >=1) { ws http://codereview.chromium.org/2808100/diff/1/7#newcode564 chrome/browser/resources/imageburner.html:564: var mainList = document.createElement('div'); This is pretty hard to read. Can you use HTML instead? http://codereview.chromium.org/2808100/diff/1/7#newcode572 chrome/browser/resources/imageburner.html:572: divTitle.style['text-align'] = 'center'; style.textAlign http://codereview.chromium.org/2808100/diff/1/7#newcode572 chrome/browser/resources/imageburner.html:572: divTitle.style['text-align'] = 'center'; Move to style sheet http://codereview.chromium.org/2808100/diff/1/7#newcode573 chrome/browser/resources/imageburner.html:573: divTitle.innerHTML = kListTitle; is kListTitle an HTML string? If not use textContent http://codereview.chromium.org/2808100/diff/1/7#newcode625 chrome/browser/resources/imageburner.html:625: <body onload='load();' onselectstart='return false'> do not use window.onload. Call your init function when the DOM is ready document.addEventListener('DOMContentReady' function() { // init code }); http://codereview.chromium.org/2808100/diff/1/7#newcode625 chrome/browser/resources/imageburner.html:625: <body onload='load();' onselectstart='return false'> HTML attributes should use double quotes http://codereview.chromium.org/2808100/diff/1/7#newcode625 chrome/browser/resources/imageburner.html:625: <body onload='load();' onselectstart='return false'> If you want to prevent selection use CSS. body { -webkit-user-select: none; }
http://codereview.chromium.org/2808100/diff/1/7 File chrome/browser/resources/imageburner.html (right): http://codereview.chromium.org/2808100/diff/1/7#newcode1 chrome/browser/resources/imageburner.html:1: <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"> On 2010/08/03 22:50:46, arv wrote: > <!DOCTYPE html> Done. http://codereview.chromium.org/2808100/diff/1/7#newcode4 chrome/browser/resources/imageburner.html:4: <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> On 2010/08/03 22:50:46, arv wrote: > <met charset="UTF-8"> Done. http://codereview.chromium.org/2808100/diff/1/7#newcode6 chrome/browser/resources/imageburner.html:6: <style type="text/css"> On 2010/08/03 22:50:46, arv wrote: > <style> Done. http://codereview.chromium.org/2808100/diff/1/7#newcode7 chrome/browser/resources/imageburner.html:7: div.header { On 2010/08/03 22:50:46, arv wrote: > looks like a header to me. > > Can you use Hx? Done. http://codereview.chromium.org/2808100/diff/1/7#newcode16 chrome/browser/resources/imageburner.html:16: box-sizing: border-box; On 2010/08/03 22:50:46, arv wrote: > box-sizing is not yet supported. Done. http://codereview.chromium.org/2808100/diff/1/7#newcode22 chrome/browser/resources/imageburner.html:22: border-bottom-color: #999; On 2010/08/03 22:50:46, arv wrote: > It seems like you can simplify this quite a lot > > border: 1px solid #99l > border-top: 0; Done. http://codereview.chromium.org/2808100/diff/1/7#newcode43 chrome/browser/resources/imageburner.html:43: width: 90%; On 2010/08/03 22:50:46, arv wrote: > This seems pretty arbitrary Done. http://codereview.chromium.org/2808100/diff/1/7#newcode64 chrome/browser/resources/imageburner.html:64: margin-left: -44px; On 2010/08/03 22:50:46, arv wrote: > -webkit-margin-start? Done. http://codereview.chromium.org/2808100/diff/1/7#newcode80 chrome/browser/resources/imageburner.html:80: align: right; On 2010/08/03 22:50:46, arv wrote: > align is not a valid CSS property > > Did you mean text-align? > > text-align: end Done. http://codereview.chromium.org/2808100/diff/1/7#newcode87 chrome/browser/resources/imageburner.html:87: margin-left: 0; On 2010/08/03 22:50:46, arv wrote: > -webkit-margin-start > -webkit-margin-end Done. http://codereview.chromium.org/2808100/diff/1/7#newcode92 chrome/browser/resources/imageburner.html:92: -webkit-transition: opacity 0.2s ease-out ; On 2010/08/03 22:50:46, arv wrote: > .15s is the standard transition time in chrome Done. http://codereview.chromium.org/2808100/diff/1/7#newcode105 chrome/browser/resources/imageburner.html:105: -webkit-transition: opacity 0.0s ease-out ; On 2010/08/03 22:50:46, arv wrote: > transition: none? Done. http://codereview.chromium.org/2808100/diff/1/7#newcode109 chrome/browser/resources/imageburner.html:109: opacity: 1.0; On 2010/08/03 22:50:46, arv wrote: > opacity: 1; Done. http://codereview.chromium.org/2808100/diff/1/7#newcode132 chrome/browser/resources/imageburner.html:132: margin-left: -22px; On 2010/08/03 22:50:46, arv wrote: > -webkit-margin-start Done. http://codereview.chromium.org/2808100/diff/1/7#newcode158 chrome/browser/resources/imageburner.html:158: -webkit-transition: color 1.0s ease-out ; On 2010/08/03 22:50:46, arv wrote: > Isn't 1s is too slow? Done. http://codereview.chromium.org/2808100/diff/1/7#newcode253 chrome/browser/resources/imageburner.html:253: div.progressBarRight { On 2010/08/03 22:50:46, arv wrote: > How about using the <progress> element :-) Done. http://codereview.chromium.org/2808100/diff/1/7#newcode295 chrome/browser/resources/imageburner.html:295: height:50%; On 2010/08/03 22:50:46, arv wrote: > Missing whitespace after : Done. http://codereview.chromium.org/2808100/diff/1/7#newcode297 chrome/browser/resources/imageburner.html:297: padding-top: 1%; On 2010/08/03 22:50:46, arv wrote: > What are trying to achieve here? make some space between displayed text and div border http://codereview.chromium.org/2808100/diff/1/7#newcode317 chrome/browser/resources/imageburner.html:317: border-top: solid 1px #7289E2; On 2010/08/03 22:50:46, arv wrote: > 1px solid #7289e2 for consistency Done. http://codereview.chromium.org/2808100/diff/1/7#newcode337 chrome/browser/resources/imageburner.html:337: var rootsDiv = null; On 2010/08/03 22:50:46, arv wrote: > do you really need to assign null here? guess not http://codereview.chromium.org/2808100/diff/1/7#newcode340 chrome/browser/resources/imageburner.html:340: var kListTitle; On 2010/08/03 22:50:46, arv wrote: > const LIST_TITLE? Done. http://codereview.chromium.org/2808100/diff/1/7#newcode344 chrome/browser/resources/imageburner.html:344: function PromtUserDownloadFinished() { On 2010/08/03 22:50:46, arv wrote: > promptuserDownloadFinished > > Only class names and enums should be capitalized Done. http://codereview.chromium.org/2808100/diff/1/7#newcode346 chrome/browser/resources/imageburner.html:346: var answer = confirm(localStrings.getString('burnConfirmText1')+ On 2010/08/03 22:50:46, arv wrote: > ws around binops Done. http://codereview.chromium.org/2808100/diff/1/7#newcode372 chrome/browser/resources/imageburner.html:372: var main = $('rootsColumn'); On 2010/08/03 22:50:46, arv wrote: > tabs? Done. http://codereview.chromium.org/2808100/diff/1/7#newcode379 chrome/browser/resources/imageburner.html:379: if(currentlySelectedItem) On 2010/08/03 22:50:46, arv wrote: > if (...) Done. http://codereview.chromium.org/2808100/diff/1/7#newcode387 chrome/browser/resources/imageburner.html:387: return function() { On 2010/08/03 22:50:46, arv wrote: > fix indentation Done. http://codereview.chromium.org/2808100/diff/1/7#newcode522 chrome/browser/resources/imageburner.html:522: link.onclick = getFunctionForItem(path, element.id); On 2010/08/03 22:50:46, arv wrote: > Just inline the function here. Done. http://codereview.chromium.org/2808100/diff/1/7#newcode526 chrome/browser/resources/imageburner.html:526: icon.className = 'icon iconfolder'; On 2010/08/03 22:50:46, arv wrote: > ws Done. http://codereview.chromium.org/2808100/diff/1/7#newcode556 chrome/browser/resources/imageburner.html:556: while (list.childNodes.length >=1) { On 2010/08/03 22:50:46, arv wrote: > ws Done. http://codereview.chromium.org/2808100/diff/1/7#newcode564 chrome/browser/resources/imageburner.html:564: var mainList = document.createElement('div'); On 2010/08/03 22:50:46, arv wrote: > This is pretty hard to read. Can you use HTML instead? Done. http://codereview.chromium.org/2808100/diff/1/7#newcode572 chrome/browser/resources/imageburner.html:572: divTitle.style['text-align'] = 'center'; On 2010/08/03 22:50:46, arv wrote: > style.textAlign Done. http://codereview.chromium.org/2808100/diff/1/7#newcode573 chrome/browser/resources/imageburner.html:573: divTitle.innerHTML = kListTitle; On 2010/08/03 22:50:46, arv wrote: > is kListTitle an HTML string? If not use textContent Done. http://codereview.chromium.org/2808100/diff/1/7#newcode573 chrome/browser/resources/imageburner.html:573: divTitle.innerHTML = kListTitle; On 2010/08/03 22:50:46, arv wrote: > is kListTitle an HTML string? If not use textContent Done. http://codereview.chromium.org/2808100/diff/1/7#newcode625 chrome/browser/resources/imageburner.html:625: <body onload='load();' onselectstart='return false'> On 2010/08/03 22:50:46, arv wrote: > do not use window.onload. Call your init function when the DOM is ready > > document.addEventListener('DOMContentReady' function() { > // init code > }); Done. http://codereview.chromium.org/2808100/diff/1/7#newcode625 chrome/browser/resources/imageburner.html:625: <body onload='load();' onselectstart='return false'> On 2010/08/03 22:50:46, arv wrote: > If you want to prevent selection use CSS. > > body { > -webkit-user-select: none; > } Done. http://codereview.chromium.org/2808100/diff/1/7#newcode625 chrome/browser/resources/imageburner.html:625: <body onload='load();' onselectstart='return false'> On 2010/08/03 22:50:46, arv wrote: > HTML attributes should use double quotes Done.
I haven't looked at the C++ code yet but I took a quick glance and I think you should familiarize your self with our style guides. I don't see any flags for this. Is this really ready to be shipped as part of Chrome? I would at least expect all new features to be behind a flag until they are polished enough to be shipped on the stable channel. http://codereview.chromium.org/2808100/diff/10001/4007 File chrome/browser/resources/imageburner.html (right): http://codereview.chromium.org/2808100/diff/10001/4007#newcode15 chrome/browser/resources/imageburner.html:15: width: 100%; I doubt you need this one. auto, which is default behaves better in most cases. http://codereview.chromium.org/2808100/diff/10001/4007#newcode16 chrome/browser/resources/imageburner.html:16: left: 0; left and top are ignored for non positioned elements http://codereview.chromium.org/2808100/diff/10001/4007#newcode41 chrome/browser/resources/imageburner.html:41: width: 90%; Maybe use margins and width auto? Arbitrary percentages as these are usually a bad idea http://codereview.chromium.org/2808100/diff/10001/4007#newcode44 chrome/browser/resources/imageburner.html:44: div.devicelist { skip the tag name for selectors like these http://codereview.chromium.org/2808100/diff/10001/4007#newcode45 chrome/browser/resources/imageburner.html:45: margin: 0; margin and padding are by default 0 on a div. http://codereview.chromium.org/2808100/diff/10001/4007#newcode54 chrome/browser/resources/imageburner.html:54: display: inline; this seems strange. inline and float; one of them should go away http://codereview.chromium.org/2808100/diff/10001/4007#newcode67 chrome/browser/resources/imageburner.html:67: float: right; float is useless when position is set to absolute. http://codereview.chromium.org/2808100/diff/10001/4007#newcode79 chrome/browser/resources/imageburner.html:79: -webkit-transition: opacity 0.15s ease-out ; remove ws before ; http://codereview.chromium.org/2808100/diff/10001/4007#newcode90 chrome/browser/resources/imageburner.html:90: -webkit-transition: none ; ws http://codereview.chromium.org/2808100/diff/10001/4007#newcode102 chrome/browser/resources/imageburner.html:102: position: relative; Why do you need position relative here and a lot of other places. There are 2 use cases for position: relative 1. The left/top is used to offset the position of the element 2. The element contains absolutely positioned elements. http://codereview.chromium.org/2808100/diff/10001/4007#newcode121 chrome/browser/resources/imageburner.html:121: div.devicerow input.name { Why is there a second rule for "div.devicerow input.name" http://codereview.chromium.org/2808100/diff/10001/4007#newcode176 chrome/browser/resources/imageburner.html:176: progress { I would keep the default look and feel for this. If that doesn't look like what we want it in CrOS, then we should fix the theme for CrOS. http://codereview.chromium.org/2808100/diff/10001/4007#newcode177 chrome/browser/resources/imageburner.html:177: margin-left:4%; percentage margins makes things not line up correctly http://codereview.chromium.org/2808100/diff/10001/4007#newcode178 chrome/browser/resources/imageburner.html:178: margin-right:0; ws and -webkit-margin-end http://codereview.chromium.org/2808100/diff/10001/4007#newcode188 chrome/browser/resources/imageburner.html:188: html[dir='rtl'] progress.progressBar { remove this one and use direction aware margin properties http://codereview.chromium.org/2808100/diff/10001/4007#newcode194 chrome/browser/resources/imageburner.html:194: height: 15%; don't use percentages http://codereview.chromium.org/2808100/diff/10001/4007#newcode201 chrome/browser/resources/imageburner.html:201: padding-left: auto; -webkit-padding-start and end http://codereview.chromium.org/2808100/diff/10001/4007#newcode255 chrome/browser/resources/imageburner.html:255: //this could be done nicer fix indentation Capitalize comments and end with a period http://codereview.chromium.org/2808100/diff/10001/4007#newcode258 chrome/browser/resources/imageburner.html:258: if(answer) { if (expr) { or you could use ? : here chrome.send(answer ? 'burnImage' : 'cancelBurnImage', []) http://codereview.chromium.org/2808100/diff/10001/4007#newcode287 chrome/browser/resources/imageburner.html:287: for (var x=0; x < results.length; x++) { fix formatting http://codereview.chromium.org/2808100/diff/10001/4007#newcode288 chrome/browser/resources/imageburner.html:288: if(addedRoots[results[x].title] == undefined) { if (!addedRoots[results[x].title]) or maybe if (!('title' in results[x])) { http://codereview.chromium.org/2808100/diff/10001/4007#newcode292 chrome/browser/resources/imageburner.html:292: SelectItem(element.id, results[x].path); lowercase SelectItem http://codereview.chromium.org/2808100/diff/10001/4007#newcode304 chrome/browser/resources/imageburner.html:304: function selectItem(elementid, path) { elementId http://codereview.chromium.org/2808100/diff/10001/4007#newcode342 chrome/browser/resources/imageburner.html:342: if (element.children[i].className == 'statusText') { switch? http://codereview.chromium.org/2808100/diff/10001/4007#newcode344 chrome/browser/resources/imageburner.html:344: }else if (element.children[i].className == 'progressBar') { } else http://codereview.chromium.org/2808100/diff/10001/4007#newcode350 chrome/browser/resources/imageburner.html:350: if(event == 'COMPLETE') if ( http://codereview.chromium.org/2808100/diff/10001/4007#newcode351 chrome/browser/resources/imageburner.html:351: statusTextDiv.textContent = statuses["finished"]; Please be consistent and only use single quotes http://codereview.chromium.org/2808100/diff/10001/4007#newcode354 chrome/browser/resources/imageburner.html:354: statusTextDiv.style['height'] = '100%'; style.height http://codereview.chromium.org/2808100/diff/10001/4007#newcode429 chrome/browser/resources/imageburner.html:429: link = document.createElement('div'); Should this be a button? How do I get to it using the keyboard? http://codereview.chromium.org/2808100/diff/10001/4007#newcode444 chrome/browser/resources/imageburner.html:444: var burnicon = document.createElement('div'); Should this be a button? How do I get to it using the keyboard? http://codereview.chromium.org/2808100/diff/10001/4007#newcode458 chrome/browser/resources/imageburner.html:458: if (list.hasChildNodes()) { It seems like you can skip the if here http://codereview.chromium.org/2808100/diff/10001/4007#newcode466 chrome/browser/resources/imageburner.html:466: localStrings = new LocalStrings(); fix indentation http://codereview.chromium.org/2808100/diff/10001/4007#newcode472 chrome/browser/resources/imageburner.html:472: statusMessages = new Array(); Use []
Ok, I will put this behind flag...as soon as I figure out how :) http://codereview.chromium.org/2808100/diff/10001/4007 File chrome/browser/resources/imageburner.html (right): http://codereview.chromium.org/2808100/diff/10001/4007#newcode15 chrome/browser/resources/imageburner.html:15: width: 100%; On 2010/08/05 21:05:38, arv wrote: > I doubt you need this one. auto, which is default behaves better in most cases. Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode16 chrome/browser/resources/imageburner.html:16: left: 0; On 2010/08/05 21:05:38, arv wrote: > left and top are ignored for non positioned elements Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode41 chrome/browser/resources/imageburner.html:41: width: 90%; On 2010/08/05 21:05:38, arv wrote: > Maybe use margins and width auto? Arbitrary percentages as these are usually a > bad idea Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode44 chrome/browser/resources/imageburner.html:44: div.devicelist { On 2010/08/05 21:05:38, arv wrote: > skip the tag name for selectors like these Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode45 chrome/browser/resources/imageburner.html:45: margin: 0; On 2010/08/05 21:05:38, arv wrote: > margin and padding are by default 0 on a div. Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode54 chrome/browser/resources/imageburner.html:54: display: inline; On 2010/08/05 21:05:38, arv wrote: > this seems strange. inline and float; one of them should go away Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode67 chrome/browser/resources/imageburner.html:67: float: right; On 2010/08/05 21:05:38, arv wrote: > float is useless when position is set to absolute. Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode79 chrome/browser/resources/imageburner.html:79: -webkit-transition: opacity 0.15s ease-out ; On 2010/08/05 21:05:38, arv wrote: > remove ws before ; Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode90 chrome/browser/resources/imageburner.html:90: -webkit-transition: none ; On 2010/08/05 21:05:38, arv wrote: > ws Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode102 chrome/browser/resources/imageburner.html:102: position: relative; On 2010/08/05 21:05:38, arv wrote: > Why do you need position relative here and a lot of other places. There are 2 > use cases for position: relative > > 1. The left/top is used to offset the position of the element > 2. The element contains absolutely positioned elements. Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode121 chrome/browser/resources/imageburner.html:121: div.devicerow input.name { On 2010/08/05 21:05:38, arv wrote: > Why is there a second rule for "div.devicerow input.name" the other one is for span.name, but yes, I should remove this.. Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode176 chrome/browser/resources/imageburner.html:176: progress { On 2010/08/05 21:05:38, arv wrote: > I would keep the default look and feel for this. If that doesn't look like what > we want it in CrOS, then we should fix the theme for CrOS. Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode177 chrome/browser/resources/imageburner.html:177: margin-left:4%; On 2010/08/05 21:05:38, arv wrote: > percentage margins makes things not line up correctly Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode178 chrome/browser/resources/imageburner.html:178: margin-right:0; On 2010/08/05 21:05:38, arv wrote: > ws and -webkit-margin-end Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode188 chrome/browser/resources/imageburner.html:188: html[dir='rtl'] progress.progressBar { On 2010/08/05 21:05:38, arv wrote: > remove this one and use direction aware margin properties Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode194 chrome/browser/resources/imageburner.html:194: height: 15%; On 2010/08/05 21:05:38, arv wrote: > don't use percentages Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode201 chrome/browser/resources/imageburner.html:201: padding-left: auto; On 2010/08/05 21:05:38, arv wrote: > -webkit-padding-start and end Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode255 chrome/browser/resources/imageburner.html:255: //this could be done nicer On 2010/08/05 21:05:38, arv wrote: > fix indentation > > Capitalize comments and end with a period Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode258 chrome/browser/resources/imageburner.html:258: if(answer) { On 2010/08/05 21:05:38, arv wrote: > if (expr) { > > or you could use ? : here > > chrome.send(answer ? 'burnImage' : 'cancelBurnImage', []) Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode287 chrome/browser/resources/imageburner.html:287: for (var x=0; x < results.length; x++) { On 2010/08/05 21:05:38, arv wrote: > fix formatting Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode288 chrome/browser/resources/imageburner.html:288: if(addedRoots[results[x].title] == undefined) { On 2010/08/05 21:05:38, arv wrote: > if (!addedRoots[results[x].title]) > > or maybe > > if (!('title' in results[x])) { Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode292 chrome/browser/resources/imageburner.html:292: SelectItem(element.id, results[x].path); On 2010/08/05 21:05:38, arv wrote: > lowercase SelectItem Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode304 chrome/browser/resources/imageburner.html:304: function selectItem(elementid, path) { On 2010/08/05 21:05:38, arv wrote: > elementId Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode342 chrome/browser/resources/imageburner.html:342: if (element.children[i].className == 'statusText') { On 2010/08/05 21:05:38, arv wrote: > switch? Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode344 chrome/browser/resources/imageburner.html:344: }else if (element.children[i].className == 'progressBar') { On 2010/08/05 21:05:38, arv wrote: > } else Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode350 chrome/browser/resources/imageburner.html:350: if(event == 'COMPLETE') On 2010/08/05 21:05:38, arv wrote: > if ( Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode351 chrome/browser/resources/imageburner.html:351: statusTextDiv.textContent = statuses["finished"]; On 2010/08/05 21:05:38, arv wrote: > Please be consistent and only use single quotes Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode354 chrome/browser/resources/imageburner.html:354: statusTextDiv.style['height'] = '100%'; On 2010/08/05 21:05:38, arv wrote: > style.height Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode429 chrome/browser/resources/imageburner.html:429: link = document.createElement('div'); On 2010/08/05 21:05:38, arv wrote: > Should this be a button? > > How do I get to it using the keyboard? I would leave it this way, since only thing that happens when this is clicked is highlighting div..or maybe remove onclick event for this... http://codereview.chromium.org/2808100/diff/10001/4007#newcode444 chrome/browser/resources/imageburner.html:444: var burnicon = document.createElement('div'); On 2010/08/05 21:05:38, arv wrote: > Should this be a button? > > How do I get to it using the keyboard? Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode458 chrome/browser/resources/imageburner.html:458: if (list.hasChildNodes()) { On 2010/08/05 21:05:38, arv wrote: > It seems like you can skip the if here Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode466 chrome/browser/resources/imageburner.html:466: localStrings = new LocalStrings(); On 2010/08/05 21:05:38, arv wrote: > fix indentation Done. http://codereview.chromium.org/2808100/diff/10001/4007#newcode472 chrome/browser/resources/imageburner.html:472: statusMessages = new Array(); On 2010/08/05 21:05:38, arv wrote: > Use [] Done.
http://codereview.chromium.org/2808100/diff/18001/19005 File chrome/browser/dom_ui/imageburner_ui.h (right): http://codereview.chromium.org/2808100/diff/18001/19005#newcode1 chrome/browser/dom_ui/imageburner_ui.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. please move this class to chrome/browser/chromeos/dom_ui http://codereview.chromium.org/2808100/diff/18001/19007 File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/2808100/diff/18001/19007#newcode1159 chrome/chrome_browser.gypi:1159: 'browser/dom_ui/imageburner_ui.cc', move to browser/chromeos/dom_ui http://codereview.chromium.org/2808100/diff/18001/19008 File chrome/common/url_constants.cc (right): http://codereview.chromium.org/2808100/diff/18001/19008#newcode92 chrome/common/url_constants.cc:92: const char kChromeUIImageBurnerHost[] = "imageburner"; please create kChromeUIImageBurnerURL as well (see segment above this one) http://codereview.chromium.org/2808100/diff/18001/19009 File chrome/common/url_constants.h (right): http://codereview.chromium.org/2808100/diff/18001/19009#newcode67 chrome/common/url_constants.h:67: extern const char kChromeUIImageBurnerHost[]; move this to host section below
http://codereview.chromium.org/2808100/diff/18001/19005 File chrome/browser/dom_ui/imageburner_ui.h (right): http://codereview.chromium.org/2808100/diff/18001/19005#newcode1 chrome/browser/dom_ui/imageburner_ui.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2010/08/11 20:34:05, zel wrote: > please move this class to chrome/browser/chromeos/dom_ui > Done. http://codereview.chromium.org/2808100/diff/18001/19007 File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/2808100/diff/18001/19007#newcode1159 chrome/chrome_browser.gypi:1159: 'browser/dom_ui/imageburner_ui.cc', On 2010/08/11 20:34:05, zel wrote: > move to browser/chromeos/dom_ui Done. http://codereview.chromium.org/2808100/diff/18001/19008 File chrome/common/url_constants.cc (right): http://codereview.chromium.org/2808100/diff/18001/19008#newcode92 chrome/common/url_constants.cc:92: const char kChromeUIImageBurnerHost[] = "imageburner"; On 2010/08/11 20:34:05, zel wrote: > please create kChromeUIImageBurnerURL as well (see segment above this one) Done. http://codereview.chromium.org/2808100/diff/18001/19009 File chrome/common/url_constants.h (right): http://codereview.chromium.org/2808100/diff/18001/19009#newcode67 chrome/common/url_constants.h:67: extern const char kChromeUIImageBurnerHost[]; On 2010/08/11 20:34:05, zel wrote: > move this to host section below Done.
http://codereview.chromium.org/2808100/diff/26001/27003 File chrome/browser/chromeos/dom_ui/imageburner_ui.cc (right): http://codereview.chromium.org/2808100/diff/26001/27003#newcode103 chrome/browser/chromeos/dom_ui/imageburner_ui.cc:103: #if defined(OS_CHROMEOS) remove all these http://codereview.chromium.org/2808100/diff/26001/27003#newcode155 chrome/browser/chromeos/dom_ui/imageburner_ui.cc:155: DictionaryValue* result_value = turn this into scoped_ptr<> http://codereview.chromium.org/2808100/diff/26001/27004 File chrome/browser/chromeos/dom_ui/imageburner_ui.h (right): http://codereview.chromium.org/2808100/diff/26001/27004#newcode28 chrome/browser/chromeos/dom_ui/imageburner_ui.h:28: #if defined(OS_CHROMEOS) this whole file is for chromeos only, so you don't need this here http://codereview.chromium.org/2808100/diff/26001/27004#newcode38 chrome/browser/chromeos/dom_ui/imageburner_ui.h:38: static const std::string kImageFileName = "ChromeOS-0.7.42.11-raa4a3ca2.bin.gz"; this should not be a constant. we need to read this from a known location. please ping adlr to see where this should come from http://codereview.chromium.org/2808100/diff/26001/27004#newcode63 chrome/browser/chromeos/dom_ui/imageburner_ui.h:63: #if defined(OS_CHROMEOS) remove this define http://codereview.chromium.org/2808100/diff/26001/27004#newcode77 chrome/browser/chromeos/dom_ui/imageburner_ui.h:77: #if defined(OS_CHROMEOS) this one too
http://codereview.chromium.org/2808100/diff/1/2 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/2808100/diff/1/2#newcode7738 chrome/app/generated_resources.grd:7738: Image downloaded has been terminated!! We probably don't need the !! http://codereview.chromium.org/2808100/diff/1/2#newcode7750 chrome/app/generated_resources.grd:7750: Image burning has been terminated!! Same here. http://codereview.chromium.org/2808100/diff/1/6 File chrome/browser/dom_ui/imageburner_ui.h (right): http://codereview.chromium.org/2808100/diff/1/6#newcode38 chrome/browser/dom_ui/imageburner_ui.h:38: "http://chrome-master.mtv.corp.google.com/chromeos/dev-channel/"; I feel like this should pull from somewhere else. (since this will only work on corp) http://codereview.chromium.org/2808100/diff/1/6#newcode39 chrome/browser/dom_ui/imageburner_ui.h:39: static const std::string kImageFileName = "ChromeOS-0.7.42.11-raa4a3ca2.bin.gz"; Pulling an image this way? Shouldn't it query a server somewhere to find the correct version? or something of that nature... I'd hate for us to accidentally roll a user back. (or are we shooting for doing that in a future build) http://codereview.chromium.org/2808100/diff/1/6#newcode173 chrome/browser/dom_ui/imageburner_ui.h:173: class ImageBurnUI : public HtmlDialogUI { Should this really be an html dialog? http://codereview.chromium.org/2808100/diff/26001/27003 File chrome/browser/chromeos/dom_ui/imageburner_ui.cc (right): http://codereview.chromium.org/2808100/diff/26001/27003#newcode246 chrome/browser/chromeos/dom_ui/imageburner_ui.cc:246: save_info.file_stream = linked_ptr<net::FileStream>(file_stream); Shoudl we check to make sure there's space? What if it fails due to lack of space? http://codereview.chromium.org/2808100/diff/26001/27003#newcode255 chrome/browser/chromeos/dom_ui/imageburner_ui.cc:255: active_download_item_->state() != DownloadItem::IN_PROGRESS) error case and DANGEROUS... (just in case these get marked as dangerous in the future)
Image is not pulled from static location anymore.. Also, added calling burn service and burning an image... http://codereview.chromium.org/2808100/diff/1/2 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/2808100/diff/1/2#newcode7738 chrome/app/generated_resources.grd:7738: Image downloaded has been terminated!! On 2010/08/12 23:31:50, dhg wrote: > We probably don't need the !! Done. http://codereview.chromium.org/2808100/diff/1/2#newcode7750 chrome/app/generated_resources.grd:7750: Image burning has been terminated!! On 2010/08/12 23:31:50, dhg wrote: > Same here. Done. http://codereview.chromium.org/2808100/diff/1/6 File chrome/browser/dom_ui/imageburner_ui.h (right): http://codereview.chromium.org/2808100/diff/1/6#newcode38 chrome/browser/dom_ui/imageburner_ui.h:38: "http://chrome-master.mtv.corp.google.com/chromeos/dev-channel/"; On 2010/08/12 23:31:50, dhg wrote: > I feel like this should pull from somewhere else. (since this will only work on > corp) it will be done from somewhere else, but the location is stil unknown.. from mail from sumitg: Our final location from where we will download the images haven't been finalized yet, but you can use any test location for now. Done. http://codereview.chromium.org/2808100/diff/1/6#newcode39 chrome/browser/dom_ui/imageburner_ui.h:39: static const std::string kImageFileName = "ChromeOS-0.7.42.11-raa4a3ca2.bin.gz"; On 2010/08/12 23:31:50, dhg wrote: > Pulling an image this way? Shouldn't it query a server somewhere to find the > correct version? or something of that nature... I'd hate for us to accidentally > roll a user back. (or are we shooting for doing that in a future build) Done. http://codereview.chromium.org/2808100/diff/1/6#newcode173 chrome/browser/dom_ui/imageburner_ui.h:173: class ImageBurnUI : public HtmlDialogUI { On 2010/08/12 23:31:50, dhg wrote: > Should this really be an html dialog? guess not... http://codereview.chromium.org/2808100/diff/26001/27003 File chrome/browser/chromeos/dom_ui/imageburner_ui.cc (right): http://codereview.chromium.org/2808100/diff/26001/27003#newcode103 chrome/browser/chromeos/dom_ui/imageburner_ui.cc:103: #if defined(OS_CHROMEOS) On 2010/08/12 21:27:58, zel wrote: > remove all these Done. http://codereview.chromium.org/2808100/diff/26001/27003#newcode155 chrome/browser/chromeos/dom_ui/imageburner_ui.cc:155: DictionaryValue* result_value = On 2010/08/12 21:27:58, zel wrote: > turn this into scoped_ptr<> Done. http://codereview.chromium.org/2808100/diff/26001/27003#newcode246 chrome/browser/chromeos/dom_ui/imageburner_ui.cc:246: save_info.file_stream = linked_ptr<net::FileStream>(file_stream); On 2010/08/12 23:31:51, dhg wrote: > Shoudl we check to make sure there's space? What if it fails due to lack of > space? Done. http://codereview.chromium.org/2808100/diff/26001/27003#newcode255 chrome/browser/chromeos/dom_ui/imageburner_ui.cc:255: active_download_item_->state() != DownloadItem::IN_PROGRESS) On 2010/08/12 23:31:51, dhg wrote: > error case and DANGEROUS... (just in case these get marked as dangerous in the > future) what should I do in case of DANGEROUS downloads? http://codereview.chromium.org/2808100/diff/26001/27004 File chrome/browser/chromeos/dom_ui/imageburner_ui.h (right): http://codereview.chromium.org/2808100/diff/26001/27004#newcode28 chrome/browser/chromeos/dom_ui/imageburner_ui.h:28: #if defined(OS_CHROMEOS) On 2010/08/12 21:27:58, zel wrote: > this whole file is for chromeos only, so you don't need this here Done. http://codereview.chromium.org/2808100/diff/26001/27004#newcode38 chrome/browser/chromeos/dom_ui/imageburner_ui.h:38: static const std::string kImageFileName = "ChromeOS-0.7.42.11-raa4a3ca2.bin.gz"; On 2010/08/12 21:27:58, zel wrote: > this should not be a constant. we need to read this from a known location. > please ping adlr to see where this should come from last week sumitg told me to use any test location since the final location from where images will be downloaded is not yet finalized.. http://codereview.chromium.org/2808100/diff/26001/27004#newcode63 chrome/browser/chromeos/dom_ui/imageburner_ui.h:63: #if defined(OS_CHROMEOS) On 2010/08/12 21:27:58, zel wrote: > remove this define Done. http://codereview.chromium.org/2808100/diff/26001/27004#newcode77 chrome/browser/chromeos/dom_ui/imageburner_ui.h:77: #if defined(OS_CHROMEOS) On 2010/08/12 21:27:58, zel wrote: > this one too Done.
Re:Dangerous downloads: Just handle the case by allowing the download or using the result. Just so you don't get in a state where its all out of state. On Tue, Aug 17, 2010 at 1:07 PM, <tbarzic@chromium.org> wrote: > Image is not pulled from static location anymore.. > > Also, added calling burn service and burning an image... > > > http://codereview.chromium.org/2808100/diff/1/2 > File chrome/app/generated_resources.grd (right): > > http://codereview.chromium.org/2808100/diff/1/2#newcode7738 > chrome/app/generated_resources.grd:7738: Image downloaded has been > terminated!! > On 2010/08/12 23:31:50, dhg wrote: >> >> We probably don't need the !! > > Done. > > http://codereview.chromium.org/2808100/diff/1/2#newcode7750 > chrome/app/generated_resources.grd:7750: Image burning has been > terminated!! > On 2010/08/12 23:31:50, dhg wrote: >> >> Same here. > > Done. > > http://codereview.chromium.org/2808100/diff/1/6 > File chrome/browser/dom_ui/imageburner_ui.h (right): > > http://codereview.chromium.org/2808100/diff/1/6#newcode38 > chrome/browser/dom_ui/imageburner_ui.h:38: > "http://chrome-master.mtv.corp.google.com/chromeos/dev-channel/"; > On 2010/08/12 23:31:50, dhg wrote: >> >> I feel like this should pull from somewhere else. (since this will > > only work on >> >> corp) > > it will be done from somewhere else, but the location is stil unknown.. > > from mail from sumitg: > Our final location from where we will download the images haven't been > finalized yet, but you can use any test location for now. > Done. > > http://codereview.chromium.org/2808100/diff/1/6#newcode39 > chrome/browser/dom_ui/imageburner_ui.h:39: static const std::string > kImageFileName = "ChromeOS-0.7.42.11-raa4a3ca2.bin.gz"; > On 2010/08/12 23:31:50, dhg wrote: >> >> Pulling an image this way? Shouldn't it query a server somewhere to > > find the >> >> correct version? or something of that nature... I'd hate for us to > > accidentally >> >> roll a user back. (or are we shooting for doing that in a future > > build) > > Done. > > http://codereview.chromium.org/2808100/diff/1/6#newcode173 > chrome/browser/dom_ui/imageburner_ui.h:173: class ImageBurnUI : public > HtmlDialogUI { > On 2010/08/12 23:31:50, dhg wrote: >> >> Should this really be an html dialog? > > guess not... > > http://codereview.chromium.org/2808100/diff/26001/27003 > File chrome/browser/chromeos/dom_ui/imageburner_ui.cc (right): > > http://codereview.chromium.org/2808100/diff/26001/27003#newcode103 > chrome/browser/chromeos/dom_ui/imageburner_ui.cc:103: #if > defined(OS_CHROMEOS) > On 2010/08/12 21:27:58, zel wrote: >> >> remove all these > > Done. > > http://codereview.chromium.org/2808100/diff/26001/27003#newcode155 > chrome/browser/chromeos/dom_ui/imageburner_ui.cc:155: DictionaryValue* > result_value = > On 2010/08/12 21:27:58, zel wrote: >> >> turn this into scoped_ptr<> > > Done. > > http://codereview.chromium.org/2808100/diff/26001/27003#newcode246 > chrome/browser/chromeos/dom_ui/imageburner_ui.cc:246: > save_info.file_stream = linked_ptr<net::FileStream>(file_stream); > On 2010/08/12 23:31:51, dhg wrote: >> >> Shoudl we check to make sure there's space? What if it fails due to > > lack of >> >> space? > > Done. > > http://codereview.chromium.org/2808100/diff/26001/27003#newcode255 > chrome/browser/chromeos/dom_ui/imageburner_ui.cc:255: > active_download_item_->state() != DownloadItem::IN_PROGRESS) > On 2010/08/12 23:31:51, dhg wrote: >> >> error case and DANGEROUS... (just in case these get marked as > > dangerous in the >> >> future) > > what should I do in case of DANGEROUS downloads? > > http://codereview.chromium.org/2808100/diff/26001/27004 > File chrome/browser/chromeos/dom_ui/imageburner_ui.h (right): > > http://codereview.chromium.org/2808100/diff/26001/27004#newcode28 > chrome/browser/chromeos/dom_ui/imageburner_ui.h:28: #if > defined(OS_CHROMEOS) > On 2010/08/12 21:27:58, zel wrote: >> >> this whole file is for chromeos only, so you don't need this here > > Done. > > http://codereview.chromium.org/2808100/diff/26001/27004#newcode38 > chrome/browser/chromeos/dom_ui/imageburner_ui.h:38: static const > std::string kImageFileName = "ChromeOS-0.7.42.11-raa4a3ca2.bin.gz"; > On 2010/08/12 21:27:58, zel wrote: >> >> this should not be a constant. we need to read this from a known > > location. >> >> please ping adlr to see where this should come from > > last week sumitg told me to use any test location since the final > location from where images will be downloaded is not yet finalized.. > > http://codereview.chromium.org/2808100/diff/26001/27004#newcode63 > chrome/browser/chromeos/dom_ui/imageburner_ui.h:63: #if > defined(OS_CHROMEOS) > On 2010/08/12 21:27:58, zel wrote: >> >> remove this define > > Done. > > http://codereview.chromium.org/2808100/diff/26001/27004#newcode77 > chrome/browser/chromeos/dom_ui/imageburner_ui.h:77: #if > defined(OS_CHROMEOS) > On 2010/08/12 21:27:58, zel wrote: >> >> this one too > > Done. > > http://codereview.chromium.org/2808100/show >
some style changes and fixed a bug in burn_library (burn status received from libcros should be copied)
almost there... http://codereview.chromium.org/2808100/diff/72001/73003 File chrome/browser/chromeos/cros/burn_library.cc (right): http://codereview.chromium.org/2808100/diff/72001/73003#newcode42 chrome/browser/chromeos/cros/burn_library.cc:42: NewRunnableMethod(task, &BurnLibraryTaskProxy::BurnImage, indent should be either 4 spaced from the line start or aligned with the previous param http://codereview.chromium.org/2808100/diff/72001/73003#newcode70 chrome/browser/chromeos/cros/burn_library.cc:70: BurnStatus* status_copy = new BurnStatus(); you should implement a proper copy constructor for BurnStatus class or create BurnStatus::Copy() method. Another thing to consider is to create chrome-side version of BurnStatus class that contains std::string members so you don't have to mess with bunch of delete operators like what you are doing in BurnLibraryTaskProxy::UpdateBurnStatus(). http://codereview.chromium.org/2808100/diff/72001/73003#newcode71 chrome/browser/chromeos/cros/burn_library.cc:71: char* target_path = new char[std::strlen(status.target_path)+1]; spaces around + operator http://codereview.chromium.org/2808100/diff/72001/73003#newcode71 chrome/browser/chromeos/cros/burn_library.cc:71: char* target_path = new char[std::strlen(status.target_path)+1]; since status is coming from somewhere else you should check that status.target_path and status.error are not NULL before you use them http://codereview.chromium.org/2808100/diff/72001/73003#newcode75 chrome/browser/chromeos/cros/burn_library.cc:75: char* error = new char[std::strlen(status.error)+1]; spaces around + operator http://codereview.chromium.org/2808100/diff/72001/73003#newcode86 chrome/browser/chromeos/cros/burn_library.cc:86: NewRunnableMethod(task, &BurnLibraryTaskProxy::UpdateBurnStatus, weird indentation again http://codereview.chromium.org/2808100/diff/72001/73007 File chrome/browser/chromeos/dom_ui/imageburner_ui.cc (right): http://codereview.chromium.org/2808100/diff/72001/73007#newcode190 chrome/browser/chromeos/dom_ui/imageburner_ui.cc:190: active_download_item_ = *it; are you missing break here?
http://codereview.chromium.org/2808100/diff/72001/73003 File chrome/browser/chromeos/cros/burn_library.cc (right): http://codereview.chromium.org/2808100/diff/72001/73003#newcode42 chrome/browser/chromeos/cros/burn_library.cc:42: NewRunnableMethod(task, &BurnLibraryTaskProxy::BurnImage, On 2010/08/19 07:41:57, zel wrote: > indent should be either 4 spaced from the line start or aligned with the > previous param Done. http://codereview.chromium.org/2808100/diff/72001/73003#newcode70 chrome/browser/chromeos/cros/burn_library.cc:70: BurnStatus* status_copy = new BurnStatus(); On 2010/08/19 07:41:57, zel wrote: > you should implement a proper copy constructor for BurnStatus class or create > BurnStatus::Copy() method. > > Another thing to consider is to create chrome-side version of BurnStatus class > that contains std::string members so you don't have to mess with bunch of delete > operators like what you are doing in BurnLibraryTaskProxy::UpdateBurnStatus(). Done. http://codereview.chromium.org/2808100/diff/72001/73003#newcode71 chrome/browser/chromeos/cros/burn_library.cc:71: char* target_path = new char[std::strlen(status.target_path)+1]; On 2010/08/19 07:41:57, zel wrote: > since status is coming from somewhere else you should check that > status.target_path and status.error are not NULL before you use them Done. http://codereview.chromium.org/2808100/diff/72001/73003#newcode75 chrome/browser/chromeos/cros/burn_library.cc:75: char* error = new char[std::strlen(status.error)+1]; On 2010/08/19 07:41:57, zel wrote: > spaces around + operator Done. http://codereview.chromium.org/2808100/diff/72001/73003#newcode86 chrome/browser/chromeos/cros/burn_library.cc:86: NewRunnableMethod(task, &BurnLibraryTaskProxy::UpdateBurnStatus, On 2010/08/19 07:41:57, zel wrote: > weird indentation again Done. http://codereview.chromium.org/2808100/diff/72001/73007 File chrome/browser/chromeos/dom_ui/imageburner_ui.cc (right): http://codereview.chromium.org/2808100/diff/72001/73007#newcode190 chrome/browser/chromeos/dom_ui/imageburner_ui.cc:190: active_download_item_ = *it; On 2010/08/19 07:41:57, zel wrote: > are you missing break here? Done.
LGTM
please don't submit until you get a clean trybot run - you need to explicitly kick it off with gcl try your_changelist
had to resolve some conflicts...
LGTM
http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... chrome/app/generated_resources.grd:7864: <message name="IDS_IMAGEBURN_CONFIM_BURN1" desc="Ask if user wants to burn image to stateful device (part one)"> Nit: CONFIM -> CONFIRM http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... chrome/app/generated_resources.grd:7865: Are you sure you want to burn image to folowing device: following is spelled incorrectly. This sentence is also awkward (it's missing articles). It would probably read better as, "Are you sure you want to burn the image to the following device:". http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... chrome/app/generated_resources.grd:7873: <message name="IDS_IMAGEBURN_CONFIM_BURN2" desc="Ask if user wants to burn image to stateful device (part two)"> Nit: CONFIM -> CONFIRM http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... chrome/app/generated_resources.grd:7880: Image burnt successfully! burnt -> burned for American English. See http://www.grammarist.com/usage/burned-burnt/ . http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... chrome/app/generated_resources.grd:7910: Image burn complete. Why do some progress messages end in periods and some don't? We should be consistent. http://codereview.chromium.org/2808100/diff/84001/chrome/browser/chromeos/dom... File chrome/browser/chromeos/dom_ui/imageburner_ui.cc (right): http://codereview.chromium.org/2808100/diff/84001/chrome/browser/chromeos/dom... chrome/browser/chromeos/dom_ui/imageburner_ui.cc:47: l10n_util::GetStringUTF16(IDS_IMAGEBURN_CONFIM_BURN2)); This is the wrong way to localize a sentence with a value in the middle. It should be a single message id with a placeholder. IDS_IMAGEBURN_CONFIRM_BURN1 and IDS_IMAGEBURN_CONFIRM_BURN2 should be a single string with a <ph> in the middle.
ping On 2011/08/04 17:18:44, tony wrote: > http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... > File chrome/app/generated_resources.grd (right): > > http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... > chrome/app/generated_resources.grd:7864: <message > name="IDS_IMAGEBURN_CONFIM_BURN1" desc="Ask if user wants to burn image to > stateful device (part one)"> > Nit: CONFIM -> CONFIRM > > http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... > chrome/app/generated_resources.grd:7865: Are you sure you want to burn image to > folowing device: > following is spelled incorrectly. This sentence is also awkward (it's missing > articles). It would probably read better as, "Are you sure you want to burn the > image to the following device:". > > http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... > chrome/app/generated_resources.grd:7873: <message > name="IDS_IMAGEBURN_CONFIM_BURN2" desc="Ask if user wants to burn image to > stateful device (part two)"> > Nit: CONFIM -> CONFIRM > > http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... > chrome/app/generated_resources.grd:7880: Image burnt successfully! > burnt -> burned for American English. See > http://www.grammarist.com/usage/burned-burnt/ . > > http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... > chrome/app/generated_resources.grd:7910: Image burn complete. > Why do some progress messages end in periods and some don't? We should be > consistent. > > http://codereview.chromium.org/2808100/diff/84001/chrome/browser/chromeos/dom... > File chrome/browser/chromeos/dom_ui/imageburner_ui.cc (right): > > http://codereview.chromium.org/2808100/diff/84001/chrome/browser/chromeos/dom... > chrome/browser/chromeos/dom_ui/imageburner_ui.cc:47: > l10n_util::GetStringUTF16(IDS_IMAGEBURN_CONFIM_BURN2)); > This is the wrong way to localize a sentence with a value in the middle. It > should be a single message id with a placeholder. IDS_IMAGEBURN_CONFIRM_BURN1 > and IDS_IMAGEBURN_CONFIRM_BURN2 should be a single string with a <ph> in the > middle.
On 2011/08/09 21:11:08, tony wrote: > ping > > On 2011/08/04 17:18:44, tony wrote: > > > http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... > > File chrome/app/generated_resources.grd (right): > > > > > http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... > > chrome/app/generated_resources.grd:7864: <message > > name="IDS_IMAGEBURN_CONFIM_BURN1" desc="Ask if user wants to burn image to > > stateful device (part one)"> > > Nit: CONFIM -> CONFIRM > > > > > http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... > > chrome/app/generated_resources.grd:7865: Are you sure you want to burn image > to > > folowing device: > > following is spelled incorrectly. This sentence is also awkward (it's missing > > articles). It would probably read better as, "Are you sure you want to burn > the > > image to the following device:". > > > > > http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... > > chrome/app/generated_resources.grd:7873: <message > > name="IDS_IMAGEBURN_CONFIM_BURN2" desc="Ask if user wants to burn image to > > stateful device (part two)"> > > Nit: CONFIM -> CONFIRM > > > > > http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... > > chrome/app/generated_resources.grd:7880: Image burnt successfully! > > burnt -> burned for American English. See > > http://www.grammarist.com/usage/burned-burnt/ . > > > > > http://codereview.chromium.org/2808100/diff/84001/chrome/app/generated_resour... > > chrome/app/generated_resources.grd:7910: Image burn complete. > > Why do some progress messages end in periods and some don't? We should be > > consistent. > > > > > http://codereview.chromium.org/2808100/diff/84001/chrome/browser/chromeos/dom... > > File chrome/browser/chromeos/dom_ui/imageburner_ui.cc (right): > > > > > http://codereview.chromium.org/2808100/diff/84001/chrome/browser/chromeos/dom... > > chrome/browser/chromeos/dom_ui/imageburner_ui.cc:47: > > l10n_util::GetStringUTF16(IDS_IMAGEBURN_CONFIM_BURN2)); > > This is the wrong way to localize a sentence with a value in the middle. It > > should be a single message id with a placeholder. IDS_IMAGEBURN_CONFIRM_BURN1 > > and IDS_IMAGEBURN_CONFIRM_BURN2 should be a single string with a <ph> in the > > middle. Sorry for late response.. I will get to this tomorrow morning. Btw. most of these strings are not used anymore, I planned to remove them for M14 but it slipped from my mind. So do you want me to remove them now or do you want them to be corrected?
On 2011/08/09 21:41:54, toni wrote: > Btw. most of these strings are not used anymore, I planned to remove them for > M14 but it slipped from my mind. So do you want me to remove them now or do you > want them to be corrected? If they're not used, they should be removed. |
