Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5228004:
Switch the about:gpu implementation from an about handler to dom_ui.... (Closed)
|
Created:
10 years, 1 month ago by nduca Modified:
9 years, 4 months ago CC:
chromium-reviews, ben+cc_chromium.org, pam+watch_chromium.org, apatrick_chromium, arv (Not doing code reviews), Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionSitch the about:gpu implementation from an about handler to dom_ui.
Generalize tabswitcherview slightly so it doesn't rely on div naming to
obtain its styling.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69211
Patch Set 1 #
Total comments: 2
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 10
Patch Set 4 : '' #
Total comments: 30
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 27
Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 56
Patch Set 9 : '' #
Total comments: 35
Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 7
Patch Set 12 : '' #Patch Set 13 : '' #
Total comments: 9
Patch Set 14 : '' #
Total comments: 11
Patch Set 15 : '' #Messages
Total messages: 35 (0 generated)
Eric, adding you to review because this code is c&p'd from net-internals. :)
I haven't looked through this CL in depth yet, but my initial concern is seeing the javascript files from net-internals duplicated. At a minimum, we should share the existing utility classes from net-internals by moving them to a common location (like chrome/browser/resources/shared/js). Erik, what do you think? In the past you and I discussed sharing a common implementation for these views, and I think you said there was already something more appropriate than what I had written for net-internals. Perhaps now is the time to re-visit the UI toolkit that should be used for new pages.
Anything is fine by me... I'm new to this tea party. :) On 2010/11/19 18:28:19, eroman wrote: > I haven't looked through this CL in depth yet, but my initial concern is seeing > the javascript files from net-internals duplicated. > > At a minimum, we should share the existing utility classes from net-internals by > moving them to a common location (like chrome/browser/resources/shared/js). > > Erik, what do you think? In the past you and I discussed sharing a common > implementation for these views, and I think you said there was already something > more appropriate than what I had written for net-internals. Perhaps now is the > time to re-visit the UI toolkit that should be used for new pages.
LGTM for the GPUInfo type stuff. http://codereview.chromium.org/5228004/diff/1/chrome/browser/dom_ui/gpu_ui.cc File chrome/browser/dom_ui/gpu_ui.cc (right): http://codereview.chromium.org/5228004/diff/1/chrome/browser/dom_ui/gpu_ui.cc... chrome/browser/dom_ui/gpu_ui.cc:175: /* BrowserBridge.callAsync.prepends a requestID to these messages. */ typo? "callAsync prepends"?
Eric, Matt, still looking for guidance on whether to share code with netinternals. Is that a show stopper? I'd like to land sooner than later. http://codereview.chromium.org/5228004/diff/1/chrome/browser/dom_ui/gpu_ui.cc File chrome/browser/dom_ui/gpu_ui.cc (right): http://codereview.chromium.org/5228004/diff/1/chrome/browser/dom_ui/gpu_ui.cc... chrome/browser/dom_ui/gpu_ui.cc:175: /* BrowserBridge.callAsync.prepends a requestID to these messages. */ On 2010/11/20 00:38:54, rpetterson wrote: > typo? "callAsync prepends"? Done.
It's perfectly fine with me. I assumed this was being held up by Erik, not me.
Thanks Matt. Erik? On 2010/11/23 19:01:21, Matt Menke wrote: > It's perfectly fine with me. I assumed this was being held up by Erik, not me.
While I'm here anyways...Here are a couple minor style nits with your Javascript. Just to be clear: Don't wait for a LGTM from me. Fairly new here myself, and just thought I'd check for a few of the style mistakes I've often made. http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... File chrome/browser/resources/gpu/infoview.js (right): http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... chrome/browser/resources/gpu/infoview.js:60: var subdicts = {} nit: Missing semi-colons here and on the next line. http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... chrome/browser/resources/gpu/infoview.js:64: if(typeof(v) === 'number' || typeof(v) === "string") { nit: Single quotes are preferred by Google style guidelines. Doesn't really matter much which you choose, but you should be consistent. http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... chrome/browser/resources/gpu/infoview.js:148: } nit: Missing semi-colon. This is actually closing the statement "InfoView.prototype.refresh_ = function(data) {". http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... File chrome/browser/resources/gpu/main.js (right): http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.js:63: this.nextRequestId = 0 net: Missing semi-colons, here and on the next line. http://codereview.chromium.org/5228004/diff/22002/chrome/chrome.gyp File chrome/chrome.gyp (right): http://codereview.chromium.org/5228004/diff/22002/chrome/chrome.gyp#newcode63 chrome/chrome.gyp:63: 'browser/resources/gpu_resources.grd', nit: Should be in alphabetical order.
Thanks! http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... File chrome/browser/resources/gpu/infoview.js (right): http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... chrome/browser/resources/gpu/infoview.js:60: var subdicts = {} On 2010/11/23 19:27:13, Matt Menke wrote: > nit: Missing semi-colons here and on the next line. Done. http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... chrome/browser/resources/gpu/infoview.js:64: if(typeof(v) === 'number' || typeof(v) === "string") { On 2010/11/23 19:27:13, Matt Menke wrote: > nit: Single quotes are preferred by Google style guidelines. Doesn't really > matter much which you choose, but you should be consistent. Done. http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... chrome/browser/resources/gpu/infoview.js:148: } On 2010/11/23 19:27:13, Matt Menke wrote: > nit: Missing semi-colon. This is actually closing the statement > "InfoView.prototype.refresh_ = function(data) {". Done. http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... File chrome/browser/resources/gpu/main.js (right): http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.js:63: this.nextRequestId = 0 On 2010/11/23 19:27:13, Matt Menke wrote: > net: Missing semi-colons, here and on the next line. Done. http://codereview.chromium.org/5228004/diff/22002/chrome/chrome.gyp File chrome/chrome.gyp (right): http://codereview.chromium.org/5228004/diff/22002/chrome/chrome.gyp#newcode63 chrome/chrome.gyp:63: 'browser/resources/gpu_resources.grd', On 2010/11/23 19:27:13, Matt Menke wrote: > nit: Should be in alphabetical order. Done.
Too many Erik/Erics... looking now. erik On Tue, Nov 23, 2010 at 11:39, <nduca@chromium.org> wrote: > Thanks! > > > http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... > File chrome/browser/resources/gpu/infoview.js (right): > > http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... > chrome/browser/resources/gpu/infoview.js:60: var subdicts = {} > On 2010/11/23 19:27:13, Matt Menke wrote: >> >> nit: Missing semi-colons here and on the next line. > > Done. > > http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... > chrome/browser/resources/gpu/infoview.js:64: if(typeof(v) === 'number' > || typeof(v) === "string") { > On 2010/11/23 19:27:13, Matt Menke wrote: >> >> nit: Single quotes are preferred by Google style guidelines. Â Doesn't > > really >> >> matter much which you choose, but you should be consistent. > > Done. > > http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... > chrome/browser/resources/gpu/infoview.js:148: } > On 2010/11/23 19:27:13, Matt Menke wrote: >> >> nit: Missing semi-colon. Â This is actually closing the statement >> "InfoView.prototype.refresh_ = function(data) {". > > Done. > > http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... > File chrome/browser/resources/gpu/main.js (right): > > http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... > chrome/browser/resources/gpu/main.js:63: this.nextRequestId = 0 > On 2010/11/23 19:27:13, Matt Menke wrote: >> >> net: Missing semi-colons, here and on the next line. > > Done. > > http://codereview.chromium.org/5228004/diff/22002/chrome/chrome.gyp > File chrome/chrome.gyp (right): > > http://codereview.chromium.org/5228004/diff/22002/chrome/chrome.gyp#newcode63 > chrome/chrome.gyp:63: 'browser/resources/gpu_resources.grd', > On 2010/11/23 19:27:13, Matt Menke wrote: >> >> nit: Should be in alphabetical order. > > Done. > > http://codereview.chromium.org/5228004/ >
The DOMUI JS is moving towards using cr.js which was designed for the bookmarks manager but is widely used in the new tabbed settings and it is getting some use in the NTP as well. erik On Wed, Nov 24, 2010 at 10:48, Erik Arvidsson <arv@chromium.org> wrote: > Too many Erik/Erics... looking now. > > erik > > > > On Tue, Nov 23, 2010 at 11:39, Â <nduca@chromium.org> wrote: >> Thanks! >> >> >> http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... >> File chrome/browser/resources/gpu/infoview.js (right): >> >> http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... >> chrome/browser/resources/gpu/infoview.js:60: var subdicts = {} >> On 2010/11/23 19:27:13, Matt Menke wrote: >>> >>> nit: Missing semi-colons here and on the next line. >> >> Done. >> >> http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... >> chrome/browser/resources/gpu/infoview.js:64: if(typeof(v) === 'number' >> || typeof(v) === "string") { >> On 2010/11/23 19:27:13, Matt Menke wrote: >>> >>> nit: Single quotes are preferred by Google style guidelines. Â Doesn't >> >> really >>> >>> matter much which you choose, but you should be consistent. >> >> Done. >> >> http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... >> chrome/browser/resources/gpu/infoview.js:148: } >> On 2010/11/23 19:27:13, Matt Menke wrote: >>> >>> nit: Missing semi-colon. Â This is actually closing the statement >>> "InfoView.prototype.refresh_ = function(data) {". >> >> Done. >> >> http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... >> File chrome/browser/resources/gpu/main.js (right): >> >> http://codereview.chromium.org/5228004/diff/22002/chrome/browser/resources/gp... >> chrome/browser/resources/gpu/main.js:63: this.nextRequestId = 0 >> On 2010/11/23 19:27:13, Matt Menke wrote: >>> >>> net: Missing semi-colons, here and on the next line. >> >> Done. >> >> http://codereview.chromium.org/5228004/diff/22002/chrome/chrome.gyp >> File chrome/chrome.gyp (right): >> >> http://codereview.chromium.org/5228004/diff/22002/chrome/chrome.gyp#newcode63 >> chrome/chrome.gyp:63: 'browser/resources/gpu_resources.grd', >> On 2010/11/23 19:27:13, Matt Menke wrote: >>> >>> nit: Should be in alphabetical order. >> >> Done. >> >> http://codereview.chromium.org/5228004/ >> >
My main concern is that we are doing the layout manually. This is code that can be removed if you switch to using css http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... File chrome/browser/resources/gpu/index.html (right): http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/index.html:1: <html> missing doctype I'm sure you do not want to use quirks mode. it gives you nothing but pain http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/index.html:18: <div id=categoryTabHandles> ids should be dash separated id="category-tab-handles" http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/index.html:22: <div style="clear: both;"></div> Why? floats are often not what you want http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... File chrome/browser/resources/gpu/main.css (right): http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.css:8: -webkit-box-sizing: border-box; box-sizing: border-box; http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.css:9: -moz-box-sizing: border-box; remove -moz- http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.css:11: body { one empty line between each rule http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.css:17: margin-left: 10px; -webkit-margin-start http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.css:21: background: rgb(229,236,249); space after comma http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.css:30: margin-left: 30px; -webkit-margin-start http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.css:63: padding: 5px 10px 3px 10px; rtl http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.css:84: position:relative; ws http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.css:92: table.styledTable { class names should be dash separated http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... File chrome/browser/resources/gpu/main.js (right): http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.js:19: var infoView = new InfoView("infoTabContent"); Use single quotes for all js http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.js:102: throw "requestId " + requestId + " is not pending"; only throw errors http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... File chrome/browser/resources/gpu/tabswitcherview.js (right): http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/tabswitcherview.js:31: inherits(TabSwitcherView, View); Use the following pattern for inheritance (less code) function TabSwitcherView(...) { ... } TabSwitcherView.prototype = { __proto__: View.prototype, setGeometry: function(...) { ... }, ... }; http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/tabswitcherview.js:49: TabSwitcherView.prototype.show = function(isVisible) { use a setter/getter set visible(visible) { ... }, get visible() { ... }, http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/tabswitcherview.js:74: tab.getTabHandleNode().onclick = function() { Use bind tab.getTabHandleNode().onclick = this.switchToTab.bind(this, (id); http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/tabswitcherview.js:101: for (var i = 0; i < this.tabs_.length; ++i) { indexOf http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/tabswitcherview.js:124: ids.push(this.tabs_[i].id); map http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... File chrome/browser/resources/gpu/view.js (right): http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/view.js:11: function View() { Is this a copy from net internals? We should never copy code. http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/view.js:28: View.prototype.show = function(isVisible) { visible setter http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/view.js:36: View.prototype.getLeft = function() { use getters or read only value properties http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/view.js:71: this.node_ = document.getElementById(divId); Make this extend HTMLElement instead so that you do not have to keep a wrapper http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/view.js:84: this.node_.style.position = "absolute"; move to css http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/view.js:84: this.node_.style.position = "absolute"; It seems like you are using abs pos to do the layout yourself. You should use CSS for layout where possible.
I'm a bit confused how to proceed given the conflicting desires voiced by everyone to share code and also leverage bits of cr.js. A few options I see to get my feature work done whilst being a mostly good citizen: 1. Directly include ..\net_internals\*.css/js that I need in gpu 2. Move, without much modification, net_internals shared code up to shared/js. That code would still use net_internals utility functions, not cr.js 3. Move shared code up to shared, normalize it to use cr.js conventions. Write gpu_internals using the new code, but leave net_internals as-is. 4. Write something new, in shared/js/ui/ for tabbed panel in the style of cr.js Any other options? I'm open to suggestions and direction.
On Thu, Dec 2, 2010 at 17:29, <nduca@chromium.org> wrote: > I'm a bit confused how to proceed given the conflicting desires voiced by > everyone to share code and also leverage bits of cr.js. > > A few options I see to get my feature work done whilst being a mostly good > citizen: > 1. Directly include ..\net_internals\*.css/js that I need in gpu That seems acceptable in the short term. > 2. Move, without much modification, net_internals shared code up to > shared/js. > That code would still use net_internals utility functions, not cr.js I don't think that is a good idea. Then it will be even more confusing to people which to use. > 3. Move shared code up to shared, normalize it to use cr.js conventions. > Write > gpu_internals using the new code, but leave net_internals as-is. > 4. Write something new, in shared/js/ui/ for tabbed panel in the style of > cr.js I think this is probably the best long term solution. What UI components do you need? Anything besides a tab panel? -- erik
Sounds like a plan to me, I'll go hack up the short term option. Maybe we can talk offline about the long-term option; if I can get my near term fire put out, I'd be happy to help there. Right now, its just the tab panel. Gpu isn't tremendously ambitious compared to netinternls; eventually, gpu will need a timeline visualization, which I noticed is marked todo on the netinternals side as well. On 2010/12/03 18:25:48, arv wrote:
http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... File chrome/browser/resources/gpu/index.html (right): http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/index.html:1: <html> On 2010/11/24 19:07:38, arv wrote: > missing doctype > > I'm sure you do not want to use quirks mode. it gives you nothing but pain Done. http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/index.html:18: <div id=categoryTabHandles> On 2010/11/24 19:07:38, arv wrote: > ids should be dash separated > > id="category-tab-handles" Done. http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/index.html:22: <div style="clear: both;"></div> On 2010/11/24 19:07:38, arv wrote: > Why? floats are often not what you want Done. http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... File chrome/browser/resources/gpu/main.js (right): http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.js:19: var infoView = new InfoView("infoTabContent"); On 2010/11/24 19:07:38, arv wrote: > Use single quotes for all js Done. http://codereview.chromium.org/5228004/diff/13002/chrome/browser/resources/gp... chrome/browser/resources/gpu/main.js:102: throw "requestId " + requestId + " is not pending"; On 2010/11/24 19:07:38, arv wrote: > only throw errors Done.
http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals.html (right): http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:20: var g_browser = null; var browser; http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:59: // Trigger initial layout. Would it be too much to get rid of all the js layout at this point? http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:68: <body onload="load()"> why DOMContentLoaded and window.onload? http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:72: </ul> Invalid markup. This looks like some old cruft http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/browser_bridge.js (right): http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:9: * @constructor Wrong annotation http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:15: var timeStampMs = (this.timeTickOffset_ - 0) + (timeTicks - 0); Number(expr) is cleaner http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:23: function BrowserBridge() { This one should have the @constructor http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:25: chrome.send = function(messageHandler,args) { ws after comma http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:28: args.toString() + ')'); Why not? console.log('chrome.send', messageHandler, args) http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:50: if(args === undefined) { ws http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:50: if(args === undefined) { if (!args) { http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:53: var all_args = [requestId.toString(), submessage].concat(args); no underscores in js http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:62: if(this.pendingCallbacks_[requestId] === undefined) { ws http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/info_view.js (right): http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:52: inherits(InfoView, DivView); Use __proto__ etc
http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals.html (right): http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:20: var g_browser = null; On 2010/12/03 22:19:20, arv wrote: > var browser; > Done. http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:59: // Trigger initial layout. You mean, let css do the layout rather than all the setGeometry? Ideally, I'd like to land this so I can begin some logging work, but then I'm totally down with doing a CL to do layout "more gooder." On 2010/12/03 22:19:20, arv wrote: > Would it be too much to get rid of all the js layout at this point? http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:68: <body onload="load()"> hahaha thanks! On 2010/12/03 22:19:20, arv wrote: > why DOMContentLoaded and window.onload? http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:72: </ul> Holy crap, I suck. On 2010/12/03 22:19:20, arv wrote: > Invalid markup. This looks like some old cruft http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/browser_bridge.js (right): http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:15: var timeStampMs = (this.timeTickOffset_ - 0) + (timeTicks - 0); On 2010/12/03 22:19:20, arv wrote: > Number(expr) is cleaner Turns out I'm not even using this function anymore! http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:23: function BrowserBridge() { On 2010/12/03 22:19:20, arv wrote: > This one should have the @constructor Done. http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:25: chrome.send = function(messageHandler,args) { On 2010/12/03 22:19:20, arv wrote: > ws after comma Done. http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:28: args.toString() + ')'); On 2010/12/03 22:19:20, arv wrote: > Why not? > > console.log('chrome.send', messageHandler, args) Done. http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:50: if(args === undefined) { On 2010/12/03 22:19:20, arv wrote: > ws Done. http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:53: var all_args = [requestId.toString(), submessage].concat(args); On 2010/12/03 22:19:20, arv wrote: > no underscores in js Done. http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:62: if(this.pendingCallbacks_[requestId] === undefined) { On 2010/12/03 22:19:20, arv wrote: > ws Done. http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/info_view.js (right): http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:52: inherits(InfoView, DivView); On 2010/12/03 22:19:20, arv wrote: > Use __proto__ etc Done.
http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/browser_bridge.js (right): http://codereview.chromium.org/5228004/diff/61001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:15: var timeStampMs = (this.timeTickOffset_ - 0) + (timeTicks - 0); On 2010/12/03 22:49:39, nduca wrote: > On 2010/12/03 22:19:20, arv wrote: > > Number(expr) is cleaner > > Turns out I'm not even using this function anymore! Non existing code is the cleanest code ;) http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... File chrome/browser/dom_ui/gpu_internals_ui.cc (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... chrome/browser/dom_ui/gpu_internals_ui.cc:47: class GpuHTMLSource : public ChromeURLDataManager::DataSource { Should this be named GPUHTMLSource? It looks very strange to have Gpu and HTML in the name like this. Same goes for GpuUI I guess. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... chrome/browser/dom_ui/gpu_internals_ui.cc:184: if (submessage == "requestGpuInfo") { I find this strange. DOMUI RegisterMessageCallback already does the forwarding for you. I find it clearer (and more inline with exisiting code) to just register more callbacks. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... chrome/browser/dom_ui/gpu_internals_ui.cc:197: dom_ui_->CallJavascriptFunction(L"browser.onCallAsyncReply", Same goes for calling into js http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... chrome/browser/dom_ui/gpu_internals_ui.cc:210: DictionaryValue* dict = new DictionaryValue(); Who is deleting this? http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... chrome/browser/dom_ui/gpu_internals_ui.cc:240: DictionaryValue* dict = new DictionaryValue(); Who is deleting this? http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... chrome/browser/dom_ui/gpu_internals_ui.cc:267: DictionaryValue* info = new DictionaryValue(); Who is deleting this? http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals.html (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:43: for (var i = 0; i < tabIds.length; ++i) { i++ for consistency http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:68: <body onload="onLoad()"> You should have kept DOMContentLoaded instead. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/browser_bridge.js (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:17: console.log('chrome.send', messageHandler, args); No need for the branch. I think it is fine to print undefined for args. This is just for debugging anyway. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/info_view.js (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:13: * @constructor remove @constructor here http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:31: function beginRequestClientInfo() { Move out of constructor and call with self as argument? http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:44: function beginRequestGpuInfo() { same here http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:64: var html = []; This thing might have been a lot cleaner using js templates. See chrome://extensions for example http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:65: function outputObject(obj) { Move out of refresh. I don't think there is any point in having to recreate this function on every call to refresh http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:66: var all_values = true; no underscores http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:69: if(!(typeof(v) === 'number' || typeof(v) === "string")) { whitespace after if http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:69: if(!(typeof(v) === 'number' || typeof(v) === "string")) { typeof is not a function, it is an unary operator and should not have parentheses http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:69: if(!(typeof(v) === 'number' || typeof(v) === "string")) { Use single quotes http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:77: for (var k in obj) { // output values 2 spaces before // http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:80: html.push("<tr><td><strong>"); Remove strong and use a class name + css http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:80: html.push("<tr><td><strong>"); push takes var args. html.push('<tr><td>', k, ...) http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:87: html.push("<div style=\"padding-left: 8px;\">"); -webkit-padding-start and move to css http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:102: var data = {'Data exported on' : (new Date()).toLocaleString(), no ws before : http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:117: this.textDiv_.innerHTML = html.join("\n"); XSS? You are not escaping all your text and some of that text comes from the outside. It might not be an issue because the data is gotten from chrome but using innerHTML in DOMUI is discouraged. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/tabswitcherview.css (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.css:14: float: left; rtl http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.css:15: margin-left: 5px; -webkit-margin-start http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/tabswitcherview.js (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.js:26: changeClassName(document.getElementById(tabHandleDivId), element.classList.add? http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.js:33: remove empty line http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.js:130: if(g_browser && g_browser.checkForUpdatedInfo) ws http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.js:130: if(g_browser && g_browser.checkForUpdatedInfo) Didn't you rename g_browser?
Thanks for all the comments, and for bearing with me as I get up to speed on this. I'm off to take a look at jstemplates; in the meantime, you'll see I have few clarification questions on your feedback. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... File chrome/browser/dom_ui/gpu_internals_ui.cc (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... chrome/browser/dom_ui/gpu_internals_ui.cc:47: class GpuHTMLSource : public ChromeURLDataManager::DataSource { I hear you. This is based on the rest of the Chrome GPU code, which uses Gpu rather than GPU, on class names. GpuProcessHost or GpuThread, etc. On 2010/12/06 19:45:57, arv wrote: http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... chrome/browser/dom_ui/gpu_internals_ui.cc:184: if (submessage == "requestGpuInfo") { I'm fine doing that, but question though first. I wrote this onCallAsync stuff to clean up js<->c++ interactions that are of the form "asynchronously get me value X." Without this code, you have to have the handler function statically know where to send the reply. This leads to goo when you want to use one function in two places, or when you want your callback to be a simple trampoline, such as is the case in info_view. Thoughts? I admit, this may be more an issue of me programming in a non-web-ish way than anything else... On 2010/12/06 19:45:57, arv wrote: > I find this strange. DOMUI RegisterMessageCallback already does the forwarding > for you. I find it clearer (and more inline with exisiting code) to just > register more callbacks. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... chrome/browser/dom_ui/gpu_internals_ui.cc:210: DictionaryValue* dict = new DictionaryValue(); OnCallAsync, line 200, "delete ret" after we've sent the dictionary back to javascript. Do we have an OwnPtr-type abstraction in the browser to make this clearer? On 2010/12/06 19:45:57, arv wrote: > Who is deleting this? http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... chrome/browser/dom_ui/gpu_internals_ui.cc:240: DictionaryValue* dict = new DictionaryValue(); Caller. On 2010/12/06 19:45:57, arv wrote: > Who is deleting this? http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... chrome/browser/dom_ui/gpu_internals_ui.cc:267: DictionaryValue* info = new DictionaryValue(); Caller. On 2010/12/06 19:45:57, arv wrote: > Who is deleting this? http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals.html (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:43: for (var i = 0; i < tabIds.length; ++i) { On 2010/12/06 19:45:57, arv wrote: > i++ for consistency Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:68: <body onload="onLoad()"> /me is n00b. Thanks. On 2010/12/06 19:45:57, arv wrote: > You should have kept DOMContentLoaded instead. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/browser_bridge.js (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/browser_bridge.js:17: console.log('chrome.send', messageHandler, args); On 2010/12/06 19:45:57, arv wrote: > No need for the branch. I think it is fine to print undefined for args. This is > just for debugging anyway. Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/info_view.js (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:13: * @constructor On 2010/12/06 19:45:57, arv wrote: > remove @constructor here Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:31: function beginRequestClientInfo() { On 2010/12/06 19:45:57, arv wrote: > Move out of constructor and call with self as argument? Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:44: function beginRequestGpuInfo() { On 2010/12/06 19:45:57, arv wrote: > same here Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:64: var html = []; I'll go take a peek at this and see if I can switch to that approach instead. Sending out this round of replies first to get your feedback. On 2010/12/06 19:45:57, arv wrote: > This thing might have been a lot cleaner using js templates. See > chrome://extensions for example http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:65: function outputObject(obj) { On 2010/12/06 19:45:57, arv wrote: > Move out of refresh. I don't think there is any point in having to recreate this > function on every call to refresh Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:65: function outputObject(obj) { On 2010/12/06 19:45:57, arv wrote: > Move out of refresh. I don't think there is any point in having to recreate this > function on every call to refresh Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:66: var all_values = true; On 2010/12/06 19:45:57, arv wrote: > no underscores Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:69: if(!(typeof(v) === 'number' || typeof(v) === "string")) { On 2010/12/06 19:45:57, arv wrote: > whitespace after if Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:80: html.push("<tr><td><strong>"); Going to skip the css-ization until I can look at jstemplate. On 2010/12/06 19:45:57, arv wrote: > push takes var args. > > html.push('<tr><td>', k, ...) Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:87: html.push("<div style=\"padding-left: 8px;\">"); On 2010/12/06 19:45:57, arv wrote: > -webkit-padding-start and move to css Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:102: var data = {'Data exported on' : (new Date()).toLocaleString(), On 2010/12/06 19:45:57, arv wrote: > no ws before : Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:117: this.textDiv_.innerHTML = html.join("\n"); Hopefully I can get out of this trap with jst. On 2010/12/06 19:45:57, arv wrote: > XSS? You are not escaping all your text and some of that text comes from the > outside. It might not be an issue because the data is gotten from chrome but > using innerHTML in DOMUI is discouraged. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/tabswitcherview.css (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.css:14: float: left; On 2010/12/06 19:45:57, arv wrote: > rtl Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.css:15: margin-left: 5px; On 2010/12/06 19:45:57, arv wrote: > -webkit-margin-start Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/tabswitcherview.js (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.js:26: changeClassName(document.getElementById(tabHandleDivId), On 2010/12/06 19:45:57, arv wrote: > element.classList.add? Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.js:33: On 2010/12/06 19:45:57, arv wrote: > remove empty line Done. http://codereview.chromium.org/5228004/diff/82001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.js:130: if(g_browser && g_browser.checkForUpdatedInfo) I did for gpu_internals, but gpu_internals browserBridge != NetInternals g_browser. If there's desire is to change it there as well, I can, but I'd suggest a separate CL since its netinternals only. This conditional keeps NI working without that more sweeping change. On 2010/12/06 19:45:57, arv wrote: > Didn't you rename g_browser?
http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... File chrome/browser/dom_ui/gpu_internals_ui.cc (right): http://codereview.chromium.org/5228004/diff/82001/chrome/browser/dom_ui/gpu_i... chrome/browser/dom_ui/gpu_internals_ui.cc:184: if (submessage == "requestGpuInfo") { On 2010/12/07 01:03:40, nduca wrote: > I'm fine doing that, but question though first. > > I wrote this onCallAsync stuff to clean up js<->c++ interactions that are of the > form "asynchronously get me value X." > > Without this code, you have to have the handler function statically know where > to send the reply. This leads to goo when you want to use one function in two > places, or when you want your callback to be a simple trampoline, such as is the > case in info_view. > > Thoughts? I admit, this may be more an issue of me programming in a non-web-ish > way than anything else... > > > On 2010/12/06 19:45:57, arv wrote: > > I find this strange. DOMUI RegisterMessageCallback already does the forwarding > > for you. I find it clearer (and more inline with exisiting code) to just > > register more callbacks. > One option would be to extend chrome.send to take an optional callback function. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals.html (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:13: -webkit-box-sizing: border-box; box-sizing: border-box; http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/info_view.css (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.css:19: border-collapse:collapse; ws http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/info_view.html (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.html:20: <td jsdisplay="!isArray(value)" jscontent="description + ':'" class="strong">title</td> How about: <td ...> <span jscontent="description">:</span> http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.html:20: <td jsdisplay="!isArray(value)" jscontent="description + ':'" class="strong">title</td> long line http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.html:20: <td jsdisplay="!isArray(value)" jscontent="description + ':'" class="strong">title</td> Can you come up with a better class name? http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/info_view.js (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:22: this.textDiv_ = document.getElementById('info-text'); Can you pass this in? Binding this global element in an instance of your class is not very useful. If you create 2 instances of InfoView there will be a conflict. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:23: this.gpuInfo_ = undefined; Most likely useless statement http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:26: var self = this; self is not used http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:44: if(data === undefined) { // try again in 250 ms whitespace after if http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:75: {description: 'Data exported', { key: value, ... } http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/util.js (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/util.js:4: function isArray(value) { Try Array.isArray(val) or val instanceof Array http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/tabswitcherview.css (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.css:20: direction: rtl; I don't know why this is here. Can you add a comment? http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.css:29: -webkit-border-top-right-radius: 8px; Remove -webkit- prefix for border radius http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.css:50: position:relative; whitespace after : http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/tabswitcherview.js (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.js:25: function TabSwitcherView(tabHandleDivId) { I find it bad practice to pass around IDs to elements when you can pass around elements directly. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/view.js (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/view.js:84: throw new Error("Element " + divId + " not found"); use single quotes
http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals.html (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals.html:13: -webkit-box-sizing: border-box; On 2010/12/08 17:55:19, arv wrote: > box-sizing: border-box; Done. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/info_view.css (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.css:19: border-collapse:collapse; On 2010/12/08 17:55:19, arv wrote: > ws Done. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/info_view.html (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.html:20: <td jsdisplay="!isArray(value)" jscontent="description + ':'" class="strong">title</td> On 2010/12/08 17:55:19, arv wrote: > Can you come up with a better class name? Done. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/info_view.js (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:22: this.textDiv_ = document.getElementById('info-text'); Not even used. Deleted. Haha. On 2010/12/08 17:55:19, arv wrote: > Can you pass this in? Binding this global element in an instance of your class > is not very useful. If you create 2 instances of InfoView there will be a > conflict. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:23: this.gpuInfo_ = undefined; On 2010/12/08 17:55:19, arv wrote: > Most likely useless statement Done. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:26: var self = this; On 2010/12/08 17:55:19, arv wrote: > self is not used Done. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:44: if(data === undefined) { // try again in 250 ms On 2010/12/08 17:55:19, arv wrote: > whitespace after if Done. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.js:75: {description: 'Data exported', On 2010/12/08 17:55:19, arv wrote: > { > key: value, > ... > } Done. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/util.js (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/util.js:4: function isArray(value) { On 2010/12/08 17:55:19, arv wrote: > Try Array.isArray(val) or val instanceof Array Done. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/tabswitcherview.css (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.css:20: direction: rtl; Umm, in a previous review you asked me to add it? Or did I misunderstand your comment? You wrote, "rtl" I have to admit to being blind wrt internationalization. On 2010/12/08 17:55:19, arv wrote: > I don't know why this is here. Can you add a comment? Done. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.css:29: -webkit-border-top-right-radius: 8px; On 2010/12/08 17:55:19, arv wrote: > Remove -webkit- prefix for border radius Done. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.css:50: position:relative; On 2010/12/08 17:55:19, arv wrote: > whitespace after : Done. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/tabswitcherview.js (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.js:25: function TabSwitcherView(tabHandleDivId) { Noted, and agreed for fresh code. The rest of NetInternals' code seems to hand around ids, e.g. the DivView constructor in this same line. I will add a switch from ids->elements to my existing todo for changing the absolute positioning in the tabswitcher/view system. On 2010/12/08 17:55:19, arv wrote: > I find it bad practice to pass around IDs to elements when you can pass around > elements directly. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/view.js (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/view.js:84: throw new Error("Element " + divId + " not found"); On 2010/12/08 17:55:19, arv wrote: > use single quotes Done.
I don't see things fixed that you replied done to. Did you forget to upload a new snapshot? erik On Wed, Dec 8, 2010 at 11:08, <nduca@chromium.org> wrote: > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > File chrome/browser/resources/gpu_internals.html (right): > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > chrome/browser/resources/gpu_internals.html:13: -webkit-box-sizing: > border-box; > On 2010/12/08 17:55:19, arv wrote: >> >> box-sizing: border-box; > > Done. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > File chrome/browser/resources/gpu_internals/info_view.css (right): > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > chrome/browser/resources/gpu_internals/info_view.css:19: > border-collapse:collapse; > On 2010/12/08 17:55:19, arv wrote: >> >> ws > > Done. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > File chrome/browser/resources/gpu_internals/info_view.html (right): > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > chrome/browser/resources/gpu_internals/info_view.html:20: <td > jsdisplay="!isArray(value)" jscontent="description + ':'" > class="strong">title</td> > On 2010/12/08 17:55:19, arv wrote: >> >> Can you come up with a better class name? > > Done. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > File chrome/browser/resources/gpu_internals/info_view.js (right): > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > chrome/browser/resources/gpu_internals/info_view.js:22: this.textDiv_ = > document.getElementById('info-text'); > Not even used. Deleted. Haha. > > On 2010/12/08 17:55:19, arv wrote: >> >> Can you pass this in? Binding this global element in an instance of > > your class >> >> is not very useful. If you create 2 instances of InfoView there will > > be a >> >> conflict. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > chrome/browser/resources/gpu_internals/info_view.js:23: this.gpuInfo_ = > undefined; > On 2010/12/08 17:55:19, arv wrote: >> >> Most likely useless statement > > Done. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > chrome/browser/resources/gpu_internals/info_view.js:26: var self = this; > On 2010/12/08 17:55:19, arv wrote: >> >> self is not used > > Done. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > chrome/browser/resources/gpu_internals/info_view.js:44: if(data === > undefined) { // try again in 250 ms > On 2010/12/08 17:55:19, arv wrote: >> >> whitespace after if > > Done. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > chrome/browser/resources/gpu_internals/info_view.js:75: {description: > 'Data exported', > On 2010/12/08 17:55:19, arv wrote: >> >> { >> Â key: value, >> Â ... >> } > > Done. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > File chrome/browser/resources/gpu_internals/util.js (right): > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > chrome/browser/resources/gpu_internals/util.js:4: function > isArray(value) { > On 2010/12/08 17:55:19, arv wrote: >> >> Try Array.isArray(val) or val instanceof Array > > Done. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > File chrome/browser/resources/net_internals/tabswitcherview.css (right): > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > chrome/browser/resources/net_internals/tabswitcherview.css:20: > direction: rtl; > Umm, in a previous review you asked me to add it? Or did I misunderstand > your comment? You wrote, "rtl" > I have to admit to being blind wrt internationalization. > > > On 2010/12/08 17:55:19, arv wrote: >> >> I don't know why this is here. Can you add a comment? > > Done. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > chrome/browser/resources/net_internals/tabswitcherview.css:29: > -webkit-border-top-right-radius: 8px; > On 2010/12/08 17:55:19, arv wrote: >> >> Remove -webkit- prefix for border radius > > Done. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > chrome/browser/resources/net_internals/tabswitcherview.css:50: > position:relative; > On 2010/12/08 17:55:19, arv wrote: >> >> whitespace after : > > Done. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > File chrome/browser/resources/net_internals/tabswitcherview.js (right): > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > chrome/browser/resources/net_internals/tabswitcherview.js:25: function > TabSwitcherView(tabHandleDivId) { > Noted, and agreed for fresh code. > > The rest of NetInternals' code seems to hand around ids, e.g. the > DivView constructor in this same line. I will add a switch from > ids->elements to my existing todo for changing the absolute positioning > in the tabswitcher/view system. > > On 2010/12/08 17:55:19, arv wrote: >> >> I find it bad practice to pass around IDs to elements when you can > > pass around >> >> elements directly. > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > File chrome/browser/resources/net_internals/view.js (right): > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > chrome/browser/resources/net_internals/view.js:84: throw new > Error("Element " + divId + " not found"); > On 2010/12/08 17:55:19, arv wrote: >> >> use single quotes > > Done. > > http://codereview.chromium.org/5228004/ >
I did indeed. One second... On 2010/12/08 19:18:29, arv wrote: > I don't see things fixed that you replied done to. Did you forget to > upload a new snapshot? > > erik > > > > On Wed, Dec 8, 2010 at 11:08, <mailto:nduca@chromium.org> wrote: > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > File chrome/browser/resources/gpu_internals.html (right): > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > chrome/browser/resources/gpu_internals.html:13: -webkit-box-sizing: > > border-box; > > On 2010/12/08 17:55:19, arv wrote: > >> > >> box-sizing: border-box; > > > > Done. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > File chrome/browser/resources/gpu_internals/info_view.css (right): > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > chrome/browser/resources/gpu_internals/info_view.css:19: > > border-collapse:collapse; > > On 2010/12/08 17:55:19, arv wrote: > >> > >> ws > > > > Done. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > File chrome/browser/resources/gpu_internals/info_view.html (right): > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > chrome/browser/resources/gpu_internals/info_view.html:20: <td > > jsdisplay="!isArray(value)" jscontent="description + ':'" > > class="strong">title</td> > > On 2010/12/08 17:55:19, arv wrote: > >> > >> Can you come up with a better class name? > > > > Done. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > File chrome/browser/resources/gpu_internals/info_view.js (right): > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > chrome/browser/resources/gpu_internals/info_view.js:22: this.textDiv_ = > > document.getElementById('info-text'); > > Not even used. Deleted. Haha. > > > > On 2010/12/08 17:55:19, arv wrote: > >> > >> Can you pass this in? Binding this global element in an instance of > > > > your class > >> > >> is not very useful. If you create 2 instances of InfoView there will > > > > be a > >> > >> conflict. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > chrome/browser/resources/gpu_internals/info_view.js:23: this.gpuInfo_ = > > undefined; > > On 2010/12/08 17:55:19, arv wrote: > >> > >> Most likely useless statement > > > > Done. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > chrome/browser/resources/gpu_internals/info_view.js:26: var self = this; > > On 2010/12/08 17:55:19, arv wrote: > >> > >> self is not used > > > > Done. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > chrome/browser/resources/gpu_internals/info_view.js:44: if(data === > > undefined) { // try again in 250 ms > > On 2010/12/08 17:55:19, arv wrote: > >> > >> whitespace after if > > > > Done. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > chrome/browser/resources/gpu_internals/info_view.js:75: {description: > > 'Data exported', > > On 2010/12/08 17:55:19, arv wrote: > >> > >> { > >> key: value, > >> ... > >> } > > > > Done. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > File chrome/browser/resources/gpu_internals/util.js (right): > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... > > chrome/browser/resources/gpu_internals/util.js:4: function > > isArray(value) { > > On 2010/12/08 17:55:19, arv wrote: > >> > >> Try Array.isArray(val) or val instanceof Array > > > > Done. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > > File chrome/browser/resources/net_internals/tabswitcherview.css (right): > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > > chrome/browser/resources/net_internals/tabswitcherview.css:20: > > direction: rtl; > > Umm, in a previous review you asked me to add it? Or did I misunderstand > > your comment? You wrote, "rtl" > > I have to admit to being blind wrt internationalization. > > > > > > On 2010/12/08 17:55:19, arv wrote: > >> > >> I don't know why this is here. Can you add a comment? > > > > Done. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > > chrome/browser/resources/net_internals/tabswitcherview.css:29: > > -webkit-border-top-right-radius: 8px; > > On 2010/12/08 17:55:19, arv wrote: > >> > >> Remove -webkit- prefix for border radius > > > > Done. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > > chrome/browser/resources/net_internals/tabswitcherview.css:50: > > position:relative; > > On 2010/12/08 17:55:19, arv wrote: > >> > >> whitespace after : > > > > Done. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > > File chrome/browser/resources/net_internals/tabswitcherview.js (right): > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > > chrome/browser/resources/net_internals/tabswitcherview.js:25: function > > TabSwitcherView(tabHandleDivId) { > > Noted, and agreed for fresh code. > > > > The rest of NetInternals' code seems to hand around ids, e.g. the > > DivView constructor in this same line. I will add a switch from > > ids->elements to my existing todo for changing the absolute positioning > > in the tabswitcher/view system. > > > > On 2010/12/08 17:55:19, arv wrote: > >> > >> I find it bad practice to pass around IDs to elements when you can > > > > pass around > >> > >> elements directly. > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > > File chrome/browser/resources/net_internals/view.js (right): > > > > > http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... > > chrome/browser/resources/net_internals/view.js:84: throw new > > Error("Element " + divId + " not found"); > > On 2010/12/08 17:55:19, arv wrote: > >> > >> use single quotes > > > > Done. > > > > http://codereview.chromium.org/5228004/ > >
http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/info_view.html (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.html:20: <td jsdisplay="!isArray(value)" jscontent="description + ':'" class="strong">title</td> On 2010/12/08 17:55:19, arv wrote: > Can you come up with a better class name? Not done http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.html:20: <td jsdisplay="!isArray(value)" jscontent="description + ':'" class="strong">title</td> On 2010/12/08 17:55:19, arv wrote: > long line Not done http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... File chrome/browser/resources/net_internals/tabswitcherview.css (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.css:1: .tab-switcher-view * { You should be able to do .tab-switcher-view { font-family: sans-serif; } since font-family is inherited. http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/ne... chrome/browser/resources/net_internals/tabswitcherview.css:20: direction: rtl; On 2010/12/08 19:08:22, nduca wrote: > Umm, in a previous review you asked me to add it? Or did I misunderstand your > comment? You wrote, "rtl" > I have to admit to being blind wrt internationalization. > > > On 2010/12/08 17:55:19, arv wrote: > > I don't know why this is here. Can you add a comment? > > Done. Anytime I see a hardcoded left/right and no handling of RTL I add that comment. The classic solution is to use direction aware properties (like -webkit-margin-start) where possible and for the other cases set the dir attribute on the html element and have a more specific rule. .tab-switcher-view li { float: left; } html[dir=rtl] .tab-switcher-view li { float: right; } This requires you to set the dir on the HTML element though. Almost all of the DOMUI pages do this using the i18n template system (like js templates but simpler) http://codereview.chromium.org/5228004/diff/116001/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals/info_view.js (right): http://codereview.chromium.org/5228004/diff/116001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/info_view.js:97: setText_ : function(outputElementId,text) { ws after comma http://codereview.chromium.org/5228004/diff/116001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/info_view.js:97: setText_ : function(outputElementId,text) { no ws before : http://codereview.chromium.org/5228004/diff/116001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/info_view.js:99: var textNode = peg.ownerDocument.createTextNode(text); peg.textContent = text; http://codereview.chromium.org/5228004/diff/116001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/info_view.js:102: return textNode; this return value is never used
Still reading your comments on rtl :) http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... File chrome/browser/resources/gpu_internals/info_view.html (right): http://codereview.chromium.org/5228004/diff/94001/chrome/browser/resources/gp... chrome/browser/resources/gpu_internals/info_view.html:20: <td jsdisplay="!isArray(value)" jscontent="description + ':'" class="strong">title</td> Woah, something is funky. Its changed to row-title here... On 2010/12/08 19:52:37, arv wrote: > On 2010/12/08 17:55:19, arv wrote: > > Can you come up with a better class name? > > Not done
Thanks for sorting me out on rtl, makes total sense now that I'm clued in. Hopefully I caught all the comments. http://codereview.chromium.org/5228004/diff/116001/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals/info_view.js (right): http://codereview.chromium.org/5228004/diff/116001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/info_view.js:97: setText_ : function(outputElementId,text) { On 2010/12/08 19:52:37, arv wrote: > no ws before : Done. http://codereview.chromium.org/5228004/diff/116001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/info_view.js:99: var textNode = peg.ownerDocument.createTextNode(text); On 2010/12/08 19:52:37, arv wrote: > peg.textContent = text; Done. http://codereview.chromium.org/5228004/diff/116001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/info_view.js:102: return textNode; On 2010/12/08 19:52:37, arv wrote: > this return value is never used Done.
http://codereview.chromium.org/5228004/diff/130002/chrome/browser/dom_ui/gpu_... File chrome/browser/dom_ui/gpu_internals_ui.cc (right): http://codereview.chromium.org/5228004/diff/130002/chrome/browser/dom_ui/gpu_... chrome/browser/dom_ui/gpu_internals_ui.cc:70: // TODO(eroman): Can we start on the IO thread to begin with? Looks like you copied this comment block from net_internals_ui.cc (but it doesn't apply here). http://codereview.chromium.org/5228004/diff/130002/chrome/browser/dom_ui/gpu_... chrome/browser/dom_ui/gpu_internals_ui.cc:109: bool is_off_the_record, indentation. http://codereview.chromium.org/5228004/diff/130002/chrome/browser/dom_ui/gpu_... chrome/browser/dom_ui/gpu_internals_ui.cc:114: static const base::StringPiece gpu_html( Why static? Seems incorrect to save a StringPiece to a static, since it is assuming that the string data backing it will still be alive. Why not just retrieve the resource on each call? http://codereview.chromium.org/5228004/diff/130002/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals.html (right): http://codereview.chromium.org/5228004/diff/130002/chrome/browser/resources/g... chrome/browser/resources/gpu_internals.html:21: <script src="net_internals/util.js"></script> one downside to this approach is I believe it means we will duplicate the bytes of these shared files in the resource pack (since both this HTML file and the net-internal one get flattened to a single file). I guess this is ok for the short term, but we will need to fix the sharing model soon and replace this with something from a truly common location (not net_internals).
http://codereview.chromium.org/5228004/diff/130002/chrome/browser/dom_ui/gpu_... File chrome/browser/dom_ui/gpu_internals_ui.cc (right): http://codereview.chromium.org/5228004/diff/130002/chrome/browser/dom_ui/gpu_... chrome/browser/dom_ui/gpu_internals_ui.cc:70: // TODO(eroman): Can we start on the IO thread to begin with? On 2010/12/09 02:46:45, eroman wrote: > Looks like you copied this comment block from net_internals_ui.cc (but it > doesn't apply here). Done. http://codereview.chromium.org/5228004/diff/130002/chrome/browser/dom_ui/gpu_... chrome/browser/dom_ui/gpu_internals_ui.cc:109: bool is_off_the_record, On 2010/12/09 02:46:45, eroman wrote: > indentation. Done. http://codereview.chromium.org/5228004/diff/130002/chrome/browser/dom_ui/gpu_... chrome/browser/dom_ui/gpu_internals_ui.cc:114: static const base::StringPiece gpu_html( This is copied from options_ui.cc, which does this same static thing. I'm not sure why its done there though, I tend to agree with you. On 2010/12/09 02:46:45, eroman wrote: > Why static? > > Seems incorrect to save a StringPiece to a static, since it is assuming that the > string data backing it will still be alive. > > Why not just retrieve the resource on each call? http://codereview.chromium.org/5228004/diff/130002/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals.html (right): http://codereview.chromium.org/5228004/diff/130002/chrome/browser/resources/g... chrome/browser/resources/gpu_internals.html:21: <script src="net_internals/util.js"></script> Agreed. Did you see the earlier discussion about a cr.js-styled tab switcher? On 2010/12/09 02:46:45, eroman wrote: > one downside to this approach is I believe it means we will duplicate the bytes > of these shared files in the resource pack (since both this HTML file and the > net-internal one get flattened to a single file). > > I guess this is ok for the short term, but we will need to fix the sharing model > soon and replace this with something from a truly common location (not > net_internals).
http://codereview.chromium.org/5228004/diff/130002/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals.html (right): http://codereview.chromium.org/5228004/diff/130002/chrome/browser/resources/g... chrome/browser/resources/gpu_internals.html:21: <script src="net_internals/util.js"></script> On 2010/12/09 17:24:54, nduca wrote: > Agreed. Did you see the earlier discussion about a cr.js-styled tab switcher? > > On 2010/12/09 02:46:45, eroman wrote: > > one downside to this approach is I believe it means we will duplicate the > bytes > > of these shared files in the resource pack (since both this HTML file and the > > net-internal one get flattened to a single file). > > > > I guess this is ok for the short term, but we will need to fix the sharing > model > > soon and replace this with something from a truly common location (not > > net_internals). > I didn't follow the entire discussion. As long as net_internals still works, and Erik is happy with the change then LGTM.
Erik? Are we good? On 2010/12/10 01:24:37, eroman wrote: > http://codereview.chromium.org/5228004/diff/130002/chrome/browser/resources/g... > File chrome/browser/resources/gpu_internals.html (right): > > http://codereview.chromium.org/5228004/diff/130002/chrome/browser/resources/g... > chrome/browser/resources/gpu_internals.html:21: <script > src="net_internals/util.js"></script> > On 2010/12/09 17:24:54, nduca wrote: > > Agreed. Did you see the earlier discussion about a cr.js-styled tab switcher? > > > > On 2010/12/09 02:46:45, eroman wrote: > > > one downside to this approach is I believe it means we will duplicate the > > bytes > > > of these shared files in the resource pack (since both this HTML file and > the > > > net-internal one get flattened to a single file). > > > > > > I guess this is ok for the short term, but we will need to fix the sharing > > model > > > soon and replace this with something from a truly common location (not > > > net_internals). > > > > I didn't follow the entire discussion. > > As long as net_internals still works, and Erik is happy with the change then > LGTM.
http://codereview.chromium.org/5228004/diff/150001/chrome/browser/dom_ui/gpu_... File chrome/browser/dom_ui/gpu_internals_ui.cc (right): http://codereview.chromium.org/5228004/diff/150001/chrome/browser/dom_ui/gpu_... chrome/browser/dom_ui/gpu_internals_ui.cc:233: dict->SetString("description", EscapeForHTML(desc)); Are we double escaping this? http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals.html (right): http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals.html:2: <html> Did you intend to set dir here? If not, your RTL css rules have no effect. <html i18n-values="dir:textdirection;"> http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals/info_view.html (right): http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/info_view.html:27: <span jscontent="description" class="row-title"></span> Yup, you are escaping this too much. Just remove all the escaping on the C++ side and use jscontent, textContent (in other words not innerHTML) and you are good. http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... File chrome/browser/resources/net_internals/index.html (right): http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... chrome/browser/resources/net_internals/index.html:1: <html> missing doctype http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... chrome/browser/resources/net_internals/index.html:1: <html> Set dir here as well? <html i18n-values="dir:textdirection;"> http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... File chrome/browser/resources/net_internals/view.js (right): http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... chrome/browser/resources/net_internals/view.js:83: if(!this.node_) ws
http://codereview.chromium.org/5228004/diff/150001/chrome/browser/dom_ui/gpu_... File chrome/browser/dom_ui/gpu_internals_ui.cc (right): http://codereview.chromium.org/5228004/diff/150001/chrome/browser/dom_ui/gpu_... chrome/browser/dom_ui/gpu_internals_ui.cc:233: dict->SetString("description", EscapeForHTML(desc)); Ah, good point. On 2010/12/10 22:19:42, arv wrote: > Are we double escaping this? http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals.html (right): http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals.html:2: <html> On 2010/12/10 22:19:42, arv wrote: > Did you intend to set dir here? If not, your RTL css rules have no effect. > > <html i18n-values="dir:textdirection;"> Done. http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals/info_view.html (right): http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/info_view.html:27: <span jscontent="description" class="row-title"></span> On 2010/12/10 22:19:42, arv wrote: > Yup, you are escaping this too much. Just remove all the escaping on the C++ > side and use jscontent, textContent (in other words not innerHTML) and you are > good. Done. http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... File chrome/browser/resources/net_internals/index.html (right): http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... chrome/browser/resources/net_internals/index.html:1: <html> I'm less sure about this ... net_internals code doesn't include any of the jstemplate_buidler flow. But, for consistency sake, I added this and things still behave correctly. So, I think we're good here. On 2010/12/10 22:19:42, arv wrote: > Set dir here as well? > > <html i18n-values="dir:textdirection;"> http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... File chrome/browser/resources/net_internals/view.js (right): http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... chrome/browser/resources/net_internals/view.js:83: if(!this.node_) On 2010/12/10 22:19:42, arv wrote: > ws Done.
Any final issues? This review is going on 3 and a half weeks long. On 2010/12/11 00:16:45, nduca wrote: > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/dom_ui/gpu_... > File chrome/browser/dom_ui/gpu_internals_ui.cc (right): > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/dom_ui/gpu_... > chrome/browser/dom_ui/gpu_internals_ui.cc:233: dict->SetString("description", > EscapeForHTML(desc)); > Ah, good point. > > On 2010/12/10 22:19:42, arv wrote: > > Are we double escaping this? > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... > File chrome/browser/resources/gpu_internals.html (right): > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... > chrome/browser/resources/gpu_internals.html:2: <html> > On 2010/12/10 22:19:42, arv wrote: > > Did you intend to set dir here? If not, your RTL css rules have no effect. > > > > <html i18n-values="dir:textdirection;"> > > Done. > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... > File chrome/browser/resources/gpu_internals/info_view.html (right): > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... > chrome/browser/resources/gpu_internals/info_view.html:27: <span > jscontent="description" class="row-title"></span> > On 2010/12/10 22:19:42, arv wrote: > > Yup, you are escaping this too much. Just remove all the escaping on the C++ > > side and use jscontent, textContent (in other words not innerHTML) and you are > > good. > > Done. > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... > File chrome/browser/resources/net_internals/index.html (right): > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... > chrome/browser/resources/net_internals/index.html:1: <html> > I'm less sure about this ... net_internals code doesn't include any of the > jstemplate_buidler flow. > > But, for consistency sake, I added this and things still behave correctly. So, I > think we're good here. > > On 2010/12/10 22:19:42, arv wrote: > > Set dir here as well? > > > > <html i18n-values="dir:textdirection;"> > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... > File chrome/browser/resources/net_internals/view.js (right): > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... > chrome/browser/resources/net_internals/view.js:83: if(!this.node_) > On 2010/12/10 22:19:42, arv wrote: > > ws > > Done.
Nope, LGTM On Dec 13, 2010 4:36 PM, <nduca@chromium.org> wrote: > Any final issues? This review is going on 3 and a half weeks long. > > On 2010/12/11 00:16:45, nduca wrote: > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/dom_ui/gpu_... >> File chrome/browser/dom_ui/gpu_internals_ui.cc (right): > > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/dom_ui/gpu_... >> chrome/browser/dom_ui/gpu_internals_ui.cc:233: >> dict->SetString("description", >> EscapeForHTML(desc)); >> Ah, good point. > >> On 2010/12/10 22:19:42, arv wrote: >> > Are we double escaping this? > > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... >> File chrome/browser/resources/gpu_internals.html (right): > > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... >> chrome/browser/resources/gpu_internals.html:2: <html> >> On 2010/12/10 22:19:42, arv wrote: >> > Did you intend to set dir here? If not, your RTL css rules have no >> effect. >> > >> > <html i18n-values="dir:textdirection;"> > >> Done. > > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... >> File chrome/browser/resources/gpu_internals/info_view.html (right): > > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/g... >> chrome/browser/resources/gpu_internals/info_view.html:27: <span >> jscontent="description" class="row-title"></span> >> On 2010/12/10 22:19:42, arv wrote: >> > Yup, you are escaping this too much. Just remove all the escaping on >> the C++ >> > side and use jscontent, textContent (in other words not innerHTML) and >> you > are >> > good. > >> Done. > > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... >> File chrome/browser/resources/net_internals/index.html (right): > > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... >> chrome/browser/resources/net_internals/index.html:1: <html> >> I'm less sure about this ... net_internals code doesn't include any of the >> jstemplate_buidler flow. > >> But, for consistency sake, I added this and things still behave >> correctly. So, > I >> think we're good here. > >> On 2010/12/10 22:19:42, arv wrote: >> > Set dir here as well? >> > >> > <html i18n-values="dir:textdirection;"> > > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... >> File chrome/browser/resources/net_internals/view.js (right): > > > http://codereview.chromium.org/5228004/diff/150001/chrome/browser/resources/n... >> chrome/browser/resources/net_internals/view.js:83: if(!this.node_) >> On 2010/12/10 22:19:42, arv wrote: >> > ws > >> Done. > > > > http://codereview.chromium.org/5228004/ |
