Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10832191:
Major revision of page cycler UI. (Closed)
|
Created:
8 years, 4 months ago by clintstaley Modified:
8 years, 2 months ago CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionMajor revision of page cycler UI. This revision does several things:
In this CL:
1. Reforms the JS UI to look more like Chrome|Settings, including look/feel and use of overlays for progress and error reporting.
2. Redesigns the UI to support maintaining on the extension side a record of captures, including their URLs and the cache state resulting from their initial capture run. Allows the user to add new captures to the record, delete unwanted ones, and run any he'd like.
Later CLs:
3. Actually implements the underlying FileSystem-using code to do this, as opposed to current mockup.
4. Adds C++-side code to allow C++ directories to be copied to and from FileSystem sandbox directories.
5. Adds means of interrupting ongoing sub-browser sessions so that captures and replays may be cancelled from UI.
6. Improves stats reporting, including nice-looking charts instead of present rather nasty line-by-line text dump of runtimes per URL.
Snapshots:
Capture tab: http://imgur.com/Jb3UG
Message overlay: http://imgur.com/afFVL
Playback tab: http://imgur.com/PUGhx
Playback w/o chosen capture: http://imgur.com/biKK1
Playback w/ no captures available: http://imgur.com/Zv4Bp
Aaron's initial mockup/spec:
https://moqups.com/aboodman/wDjKAEXR/p:aed59f081
Somewhat dated original design doc for the first version:
https://docs.google.com/a/chromium.org/document/d/1HyiPO4DBfCzLtVKOMx6rGGa7Vw_GzNZX_FObc-mWiZo/edit?hl=en_US&sai=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162059
Patch Set 1 : Major revision to cycler UI #
Total comments: 48
Patch Set 2 : Newer version with requested decomposition into separate sources #
Total comments: 46
Patch Set 3 : Fixes per jyasskin #
Total comments: 10
Patch Set 4 : Next round of fixes, per jyasskin's comments #
Total comments: 4
Patch Set 5 : Retrying with new DumpRenderTree run #Patch Set 6 : CL without binary elements (separately landed) #Patch Set 7 : Trying again....\ #Patch Set 8 : Retrying with redone cycler.zip #Patch Set 9 : And again with very latest master merge #Patch Set 10 : Interim fixes to browser_tests #Patch Set 11 : Latest minus the offending PNG file #Messages
Total messages: 40 (0 generated)
Looking forward to your thoughts, folks.
Clint, I meant for https://moqups.com/aboodman/wDjKAEXR/p:aed59f081 to be only a wireframe showing how the screens would be laid out and fit together - not a pixel-perfect mock. You were supposed to get the visual theme (fonts, colors, spacing, ec) from the other mocks I gave you: http://imgur.com/G53UX,bDGBq#0 http://imgur.com/G53UX,bDGBq#1 Is this just a first step, without the theme, or did you misunderstand?
I stole CSS directly from the Settings page per request, but misunderstood about these Imgur pages. Will fix tomorrow. Clint On 8/8/2012 5:51 PM, aa@chromium.org wrote: > Clint, I meant for https://moqups.com/aboodman/wDjKAEXR/p:aed59f081 to > be only a > wireframe showing how the screens would be laid out and fit together - > not a > pixel-perfect mock. > > You were supposed to get the visual theme (fonts, colors, spacing, ec) > from the > other mocks I gave you: > > http://imgur.com/G53UX,bDGBq#0 > http://imgur.com/G53UX,bDGBq#1 > > Is this just a first step, without the theme, or did you misunderstand? > > http://codereview.chromium.org/10832191/ >
On 2012/08/09 02:52:53, clintstaley_gmail.com wrote: > I stole CSS directly from the Settings page per request, but > misunderstood about these Imgur pages. Will fix tomorrow. > > Clint > > On 8/8/2012 5:51 PM, mailto:aa@chromium.org wrote: > > Clint, I meant for https://moqups.com/aboodman/wDjKAEXR/p:aed59f081 to > > be only a > > wireframe showing how the screens would be laid out and fit together - > > not a > > pixel-perfect mock. > > > > You were supposed to get the visual theme (fonts, colors, spacing, ec) > > from the > > other mocks I gave you: > > > > http://imgur.com/G53UX%2CbDGBq#0 > > http://imgur.com/G53UX%2CbDGBq#1 > > > > Is this just a first step, without the theme, or did you misunderstand? > > > > http://codereview.chromium.org/10832191/ > > > A little of each. What I understood was to use Settings CSS for fonts, buttons, etc., which I did pretty consistently, down to stealing all the elaborate shading for the buttons. Didn't quite get the importance of the Imgur drawings, so the layout will need adjustments and removal of those boxes. Will do this tomorrow and get something more correct up there. Apologies for the confusion -- it should be much work to correct, tho. And, I assume Cycler, and not Cyclops, is the title of the page, right?
On 2012/08/09 03:01:10, clintstaley wrote: > On 2012/08/09 02:52:53, http://clintstaley_gmail.com wrote: > > I stole CSS directly from the Settings page per request, but > > misunderstood about these Imgur pages. Will fix tomorrow. > > > > Clint > > > > On 8/8/2012 5:51 PM, mailto:aa@chromium.org wrote: > > > Clint, I meant for https://moqups.com/aboodman/wDjKAEXR/p:aed59f081 to > > > be only a > > > wireframe showing how the screens would be laid out and fit together - > > > not a > > > pixel-perfect mock. > > > > > > You were supposed to get the visual theme (fonts, colors, spacing, ec) > > > from the > > > other mocks I gave you: > > > > > > http://imgur.com/G53UX%252CbDGBq#0 > > > http://imgur.com/G53UX%252CbDGBq#1 > > > > > > Is this just a first step, without the theme, or did you misunderstand? > > > > > > http://codereview.chromium.org/10832191/ > > > > > > > A little of each. What I understood was to use Settings CSS for fonts, buttons, > etc., which I did pretty consistently, down to stealing all the elaborate > shading for the buttons. Didn't quite get the importance of the Imgur drawings, > so the layout will need adjustments and removal of those boxes. Will do this > tomorrow and get something more correct up there. Apologies for the confusion > -- it should not be much work to correct, tho. > > And, I assume Cycler, and not Cyclops, is the title of the page, right?
Yeah, Cycler seems more friendly.
I could fish up an "Eye of Sauron" icon for Cyclops if you wanted :) Clint On 8/9/2012 8:34 AM, aa@chromium.org wrote: > Yeah, Cycler seems more friendly. > > http://codereview.chromium.org/10832191/ >
Please include in your CL description a description of what the "major revision" consists of. http://codereview.chromium.org/10832191/diff/1001/chrome/browser/extensions/a... File chrome/browser/extensions/api/record/record_api.cc (right): http://codereview.chromium.org/10832191/diff/1001/chrome/browser/extensions/a... chrome/browser/extensions/api/record/record_api.cc:170: // FIX: Can't just use captureName -- gotta stick it in a temp dir I believe it's more common to use TODO(yourname) instead of "FIX" for changes you intend to make in a subsequent change. If you're using "FIX" to mark fixes you intend to make in _this_ change, then "hey, you forgot to make them." ;) http://codereview.chromium.org/10832191/diff/1001/chrome/browser/extensions/a... chrome/browser/extensions/api/record/record_api.cc:171: user_data_dir_ = FilePath::FromUTF8Unsafe(params->capture_name); What happens if the user's filesystem doesn't use UTF-8? Do extensions need to be aware that their capture_name will be used as a directory name? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... File chrome/common/extensions/api/experimental_record.json (right): http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:14: "Path to an unpacked extension to run in the session." By path, you mean something like "/home/username/foo/my_extension" or "C:\src\my_extension"? Are there any requirements on whether that extension can or must already be loaded? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:27: "description": "Unique name of the capture.", How should extensions allocate these names? What happens if the name collides with some other capture? Can two different extensions use the same name? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:50: "description": "Full path to cache directory used for capture" Is there a security risk here? What if the directory already exists? Can it be used to overwrite the user's files? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:64: "description": "Unique name of capture. Use to determine cache." How is it used to determine the cache? Presumably you mean that this has to match the name passed to a previous captureURLs call, but in that case, why not have captureURLs "return" an object representing the capture, and pass that into replayURLs instead of making the user track strings? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:85: "description": "Time in milliseconds to complete all runs." I saw there's a new performance-measuring API that uses a double number of milliseconds to capture microseconds: http://gent.ilcore.com/2012/06/better-timer-for-javascript.html. Have you considered doing the same thing here? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:90: "description": "Full multiline dump of output stats." In what format? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:96: "description": "List of errors during replay." What do errors look like? What kinds of errors should be expected? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler.html (right): http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:43: <div id='playback-tab' hidden> I'm not sure what the usual style is, but it might be more readable to comment which Javascript functions/states cause this tab to be shown. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler.js (right): http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:9: var cyclerUI = new (function () { Do you ever use 'cyclerUI'? I'm not a JS expert, but would it be cleaner to just use a closure with local variables instead of an object? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:12: * Note to reviewers: This is a (perhaps excessively?) fancy Grammar-o: "This is a fancy these functions into ..."? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:15: * still sorting out JS design, and would value your opinion on this. I'm not at all an expert on JS design, but it seems wrong to have a bunch of functions where "this" needs to be re-bound to refer to something other than the containing object. I think you should either have the functions take an argument or make them methods of cyclerUI. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:21: * @private I think being a local variable in the object closure makes this automatically private. Also, wouldn't this have to go through the Closure Compiler to make @private meaningful? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:107: * Enable the tab indciated by |tabLabel| and |tab|, the HTML elements sp: indciated http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:184: * Delete capture |name| from the local FileSystem, and update the Does this actually delete from the filesystem? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:216: chrome.experimental.record.captureURLs(captureName, urlList, Can captureURLs throw exceptions that need to be exposed to the user somehow? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:359: this.showMessage_('Test took ' + runTime + 'mS :\n' + stats, 'Ok'); I understand this is just the initial version, but performance measurements aren't very useful without a presentation of the inherent jitter and uncertainty. Have you thought about whether the current API supports good statistics on time measurements? Does it support a warmup phase and multiple measurements of the same loads? Intentionally-cold starts done by flushing caches? Etc?
New revision with some changes per Jeff's review, and the look/feel fixes Aaron requested. Aaron, if you have a moment to look at the Enum I did at the top of cycler.js, we'd be interested in your experienced-JS-dev design advice. Is there a nicer way to do that? I wanted to have enable-state represented in a concentrated enum, but it does result in a lot of binds. http://codereview.chromium.org/10832191/diff/1001/chrome/browser/extensions/a... File chrome/browser/extensions/api/record/record_api.cc (right): http://codereview.chromium.org/10832191/diff/1001/chrome/browser/extensions/a... chrome/browser/extensions/api/record/record_api.cc:170: // FIX: Can't just use captureName -- gotta stick it in a temp dir On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > I believe it's more common to use TODO(yourname) instead of "FIX" for changes > you intend to make in a subsequent change. If you're using "FIX" to mark fixes > you intend to make in _this_ change, then "hey, you forgot to make them." ;) Done. http://codereview.chromium.org/10832191/diff/1001/chrome/browser/extensions/a... chrome/browser/extensions/api/record/record_api.cc:171: user_data_dir_ = FilePath::FromUTF8Unsafe(params->capture_name); On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > What happens if the user's filesystem doesn't use UTF-8? Do extensions need to > be aware that their capture_name will be used as a directory name? Good point. I think we probably just need to be sure the capture names follow alphanumeric+_ rules. Will note for later revision. TODO(cstaley) not "FIX" :) added. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... File chrome/common/extensions/api/experimental_record.json (right): http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:14: "Path to an unpacked extension to run in the session." On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > By path, you mean something like "/home/username/foo/my_extension" or > "C:\src\my_extension"? Are there any requirements on whether that extension can > or must already be loaded? Yes, and no, respectively. Or at least, no because it's moot; the run will be in a sub-browser and the extension loaded by commandline argument. Clarified the comment a bit. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:27: "description": "Unique name of the capture.", On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > How should extensions allocate these names? What happens if the name collides > with some other capture? Can two different extensions use the same name? These are capture names, and their uniqueness will be enforced once I add the FileSystem storage on the JS side in the next revision. Not sure what you mean by allowing two extensions to use the same name ? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:50: "description": "Full path to cache directory used for capture" On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > Is there a security risk here? What if the directory already exists? Can it be > used to overwrite the user's files? Hmm. Your comment prompts me to think a little more carefully about this (as yet unimplemented) part of the interface. No security risk; just a security-impossibiity. We can't mess with a C++ side dir from the sandbox. So, this is now dropped. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:64: "description": "Unique name of capture. Use to determine cache." On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > How is it used to determine the cache? Presumably you mean that this has to > match the name passed to a previous captureURLs call, but in that case, why not > have captureURLs "return" an object representing the capture, and pass that into > replayURLs instead of making the user track strings? Such an approach was considered (returning a zip of the cache dir, to be stored on the JS side in FileSystem storage and sent back on playback). But, Aaron preferred an approach where we would write to and from FileSystem storage on the C++ side, so all the JS side needs is to track the names of captures, which are also the names of the FileSystem directories into which the capture state will be saved. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:85: "description": "Time in milliseconds to complete all runs." On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > I saw there's a new performance-measuring API that uses a double number of > milliseconds to capture microseconds: > http://gent.ilcore.com/2012/06/better-timer-for-javascript.html. Have you > considered doing the same thing here? Given the size of the times we're getting, not sure that resolution is needed. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:96: "description": "List of errors during replay." On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > What do errors look like? What kinds of errors should be expected? I can show you some examples. But I think they'd be a bit lengthy to put into a JSON description field. Should I be making these description fields longer?? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler.html (right): http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:43: <div id='playback-tab' hidden> On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > I'm not sure what the usual style is, but it might be more readable to comment > which Javascript functions/states cause this tab to be shown. I don't see a lot of comments int structural HTML, but I might be wrong. There are some (I hope) clear comments on the use of these divs in the JS code. Do those help? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler.js (right): http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:9: var cyclerUI = new (function () { On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > Do you ever use 'cyclerUI'? I'm not a JS expert, but would it be cleaner to just > use a closure with local variables instead of an object? I'm fairly new to it myself, and am just imitating what I've seen in other cases. The style I've seen is to use closure-locals where "static" data would be normal, and to use prototype and normal objects for what would be "member" data. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:12: * Note to reviewers: This is a (perhaps excessively?) fancy On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > Grammar-o: "This is a fancy these functions into ..."? Done. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:15: * still sorting out JS design, and would value your opinion on this. On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > I'm not at all an expert on JS design, but it seems wrong to have a bunch of > functions where "this" needs to be re-bound to refer to something other than the > containing object. I think you should either have the functions take an argument > or make them methods of cyclerUI. Yeah, this is what I was worried about, too. If you don't mind, I'd like to ping Aaron on this one, since I hear he's done a bit of JS in his time :). My $20 says he'll agree with you, but a side bet says he'll say "Oh, Clint's enum is a dumb way to do that, instead try <insert clever third solution neither of us have considered>". http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:21: * @private On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > I think being a local variable in the object closure makes this automatically > private. Also, wouldn't this have to go through the Closure Compiler to make > @private meaningful? It surely does. I was thinking of the @private as just documentation, and adding it to be thorough. Can drop it if you want. Got dinged in earlier CLs for not putting all the dots and crosses in the JSDocs, tho. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:107: * Enable the tab indciated by |tabLabel| and |tab|, the HTML elements On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > sp: indciated Done. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:184: * Delete capture |name| from the local FileSystem, and update the On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > Does this actually delete from the filesystem? No, like the comment says above, it's a mockup for now. Next revision will include much new FileSystem stuff. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:216: chrome.experimental.record.captureURLs(captureName, urlList, On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > Can captureURLs throw exceptions that need to be exposed to the user somehow? Not across an extension API boundary that I know of. They instead show up in "errors" parameters passed to the onCaptureDone callback. Given it's an asynchronous call, I'm not sure there is a way to do exception throwing. But, if you're asking more generally about error returns, yes, things like bad URLs, etc. are returned via |errors| and posted for the user to see. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.js:359: this.showMessage_('Test took ' + runTime + 'mS :\n' + stats, 'Ok'); On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > I understand this is just the initial version, but performance measurements > aren't very useful without a presentation of the inherent jitter and > uncertainty. Have you thought about whether the current API supports good > statistics on time measurements? Does it support a warmup phase and multiple > measurements of the same loads? Intentionally-cold starts done by flushing > caches? Etc? The design of the C++ side of this is a whole nother story, but the short version (longer one is in the cycler design docs) is that we are starting a sub-browser instance, and running it entirely from a preloaded cache that was set up by the initial capture run, so we don't have network latency variations to pollute the timing. This obviates any cache-clear testing. The repeat count option is meant to "average out" any computational timing variations, though how well it does that is worth discussing. Getting a more polished set of stats, even to the point of nice diagrams, is part of this new effort, just not part of the current CL :)
On 2012/08/10 00:33:13, clintstaley wrote: > New revision with some changes per Jeff's review, and the look/feel fixes Aaron > requested. > > Aaron, if you have a moment to look at the Enum I did at the top of cycler.js, > we'd be interested in your experienced-JS-dev design advice. Is there a nicer > way to do that? I wanted to have enable-state represented in a concentrated > enum, but it does result in a lot of binds. I agree with Jeffrey. Stepping back though, it seems like cycler.js is going to get a bit crowded if the entire app is in there, regardless of how it is structured. What about splitting it up into one class per tab, plus one class containing the tabs? (you might also have one class for the various overlays). You could also extract the HTML for the tab contents into separate HTML files, then fetch them using XMLHttpRequest and display them using innerHTML. > http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... > chrome/common/extensions/api/experimental_record.json:64: "description": "Unique > name of capture. Use to determine cache." > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > How is it used to determine the cache? Presumably you mean that this has to > > match the name passed to a previous captureURLs call, but in that case, why > not > > have captureURLs "return" an object representing the capture, and pass that > into > > replayURLs instead of making the user track strings? > > Such an approach was considered (returning a zip of the cache dir, to be stored > on the JS side in FileSystem storage and sent back on playback). But, Aaron > preferred an approach where we would write to and from FileSystem storage on the > C++ side, so all the JS side needs is to track the names of captures, which are > also the names of the FileSystem directories into which the capture state will > be saved. I read through my comments on this again, and can't really figure out why I thought this approach would be better than the zip file one. I remember you said the zip file approach was easier, and I don't think you've done anything yet that commits you one way or the other, so feel free to use the zip file one. > http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... > chrome/common/extensions/docs/examples/apps/cycler/cycler.html:43: <div > id='playback-tab' hidden> > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > I'm not sure what the usual style is, but it might be more readable to comment > > which Javascript functions/states cause this tab to be shown. > > I don't see a lot of comments int structural HTML, but I might be wrong. There > are some (I hope) clear comments on the use of these divs in the JS code. Do > those help? Disagree with Jeff here. Putting these comments in would be non-DRY. > http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... > File chrome/common/extensions/docs/examples/apps/cycler/cycler.js (right): > > http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/do... > chrome/common/extensions/docs/examples/apps/cycler/cycler.js:9: var cyclerUI = > new (function () { > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > Do you ever use 'cyclerUI'? I'm not a JS expert, but would it be cleaner to > just > > use a closure with local variables instead of an object? > > I'm fairly new to it myself, and am just imitating what I've seen in other > cases. The style I've seen is to use closure-locals where "static" data would > be normal, and to use prototype and normal objects for what would be "member" > data. Jeff is correct that there's no real need for the cyclerUI name, since it isn't used anywhere. I don't have a strong opinion. This stuff matters when you have a lot of code running in the page written by different people that might conflict. That isn't the situation here.
chrome/common/extensions/api/experimental_record.json:64: "description": > "Unique >> name of capture. Use to determine cache." >> On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: >> > How is it used to determine the cache? Presumably you mean that >> this has to >> > match the name passed to a previous captureURLs call, but in that >> case, why >> not >> > have captureURLs "return" an object representing the capture, and >> pass that >> into >> > replayURLs instead of making the user track strings? > >> Such an approach was considered (returning a zip of the cache dir, to be > stored >> on the JS side in FileSystem storage and sent back on playback). >> But, Aaron >> preferred an approach where we would write to and from FileSystem >> storage on > the >> C++ side, so all the JS side needs is to track the names of captures, >> which > are >> also the names of the FileSystem directories into which the capture >> state will >> be saved. > > I read through my comments on this again, and can't really figure out > why I > thought this approach would be better than the zip file one. I > remember you said > the zip file approach was easier, and I don't think you've done > anything yet > that commits you one way or the other, so feel free to use the zip > file one. > My understanding was that you wanted to have the normal filespace <-> Sandbox FileSystem transfer code for other potential purposes in Chrome. But, it would have been a bit of a research project, and I'm happy to do it with the zip files instead if you like. Clint
On 2012/08/10 07:44:53, clintstaley_gmail.com wrote: > chrome/common/extensions/api/experimental_record.json:64: "description": > > "Unique > >> name of capture. Use to determine cache." > >> On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > >> > How is it used to determine the cache? Presumably you mean that > >> this has to > >> > match the name passed to a previous captureURLs call, but in that > >> case, why > >> not > >> > have captureURLs "return" an object representing the capture, and > >> pass that > >> into > >> > replayURLs instead of making the user track strings? > > > >> Such an approach was considered (returning a zip of the cache dir, to be > > stored > >> on the JS side in FileSystem storage and sent back on playback). > >> But, Aaron > >> preferred an approach where we would write to and from FileSystem > >> storage on > > the > >> C++ side, so all the JS side needs is to track the names of captures, > >> which > > are > >> also the names of the FileSystem directories into which the capture > >> state will > >> be saved. > > > > I read through my comments on this again, and can't really figure out > > why I > > thought this approach would be better than the zip file one. I > > remember you said > > the zip file approach was easier, and I don't think you've done > > anything yet > > that commits you one way or the other, so feel free to use the zip > > file one. > > > > My understanding was that you wanted to have the normal filespace <-> > Sandbox FileSystem transfer code for other potential purposes in > Chrome. But, it would have been a bit of a research project, and I'm > happy to do it with the zip files instead if you like. Let's go back to zip.
I believe this latest has all comments responded to, and is decomposed into separate UI files, per Aaron's request. (Didin't decompose the HTML since it's not very large and that seemed overkill, but I did separate out the data handling, which will get significantly larger.)
http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... File chrome/common/extensions/api/experimental_record.json (right): http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:27: "description": "Unique name of the capture.", On 2012/08/10 00:33:13, clintstaley wrote: > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > How should extensions allocate these names? What happens if the name collides > > with some other capture? Can two different extensions use the same name? > > These are capture names, and their uniqueness will be enforced once I add the > FileSystem storage on the JS side in the next revision. What do you mean by "enforced"? That is, what's the consequence of violating the rule? If an extension calls captureURLs twice with the same captureName, what's the effect? > Not sure what you mean > by allowing two extensions to use the same name ? I'd like you to describe the scope of the uniqueness. If two different extensions both pass "cat" as the captureName argument to captureURLs(), what happens? What about the same extension on two different computers that are sync'ed to the same google account? What about the same extension in two different sessions on the same computer+profile? http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:64: "description": "Unique name of capture. Use to determine cache." On 2012/08/10 00:33:13, clintstaley wrote: > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > How is it used to determine the cache? Presumably you mean that this has to > > match the name passed to a previous captureURLs call, but in that case, why > not > > have captureURLs "return" an object representing the capture, and pass that > into > > replayURLs instead of making the user track strings? > > Such an approach was considered (returning a zip of the cache dir, to be stored > on the JS side in FileSystem storage and sent back on playback). But, Aaron > preferred an approach where we would write to and from FileSystem storage on the > C++ side, so all the JS side needs is to track the names of captures, which are > also the names of the FileSystem directories into which the capture state will > be saved. Can the extension get at the storage directory using any other FileSystem APIs? If so, maybe the extension should just pass a DirectoryEntry to this function? At the very least, the comment needs to say where the name comes from, and how it's used to determine the cache. On the other hand, I think Aaron said this function should just take a zip file now. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:85: "description": "Time in milliseconds to complete all runs." On 2012/08/10 00:33:13, clintstaley wrote: > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > I saw there's a new performance-measuring API that uses a double number of > > milliseconds to capture microseconds: > > http://gent.ilcore.com/2012/06/better-timer-for-javascript.html. Have you > > considered doing the same thing here? > > Given the size of the times we're getting, not sure that resolution is needed. > It almost certainly won't be needed for the next couple years, but it costs you very little to return a double from this interface instead of an int, and it makes sure you won't need to extend the interface in the future, if people do want more precise results. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:96: "description": "List of errors during replay." On 2012/08/10 00:33:13, clintstaley wrote: > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > What do errors look like? What kinds of errors should be expected? > > I can show you some examples. But I think they'd be a bit lengthy to put into a > JSON description field. Should I be making these description fields longer?? > Yes, I think so. Remember that this is all programmers have when they're trying to figure out how to use the API, and you don't want them to have to guess-and-check, since they're likely to draw the wrong conclusions from that. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/capture_tab.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/capture_tab.js:72: this.playbackTab_.updatePlaybackChoices_(); It seems odd to update this here without showing the tab. Should capture completion automatically show the playback tab? Should PlaybackTab.enable() automatically update the choices? http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler.html (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:1: <!doctype html5> Isn't this supposed to be "<!doctype html>"? http://developers.whatwg.org/syntax.html#the-doctype http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:38: class='name-combo'/> Don't use two class attributes. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:75: <input id='playback-extension' type='text' /> This should be a file control, right? Or can those not select directories? I don't see any use of the playback-browse button. How do you plan to use it? http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler_data.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_data.js:5: var CyclerData = function () { I assume you plan to save this object to some sort of persistent storage? http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_data.js:28: * Delete capture |name| from the local FileSystem, and update the Please mark the actual deletion as a TODO. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:12: * Enum for different UI states, either capture or playback. You can omit "capture or playback" since those are the names of the enum values. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:34: Object.defineProperty(this, 'currentCaptureName', Why use defineProperty instead of simple assignment? http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:38: this.playbackTab_ = new PlaybackTab(this, this.cyclerData_); This overwrites an existing variable. Which did you want to keep? http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:46: if (this.enableState != EnableState_.capture) { Where does enableState get initialized? I guess this works because it's initially 'undefined', which is != to everything? I'm not sure that's good style. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:50: this.captureTab_.enable(); Arguably, it seems like CaptureTab.enable should imply marking the label as selected. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/manifest.json (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/manifest.json:12: "128": "cycler_icon.png", I'd probably include the _128 in this icon's name. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:5: var PlaybackTab = function (cyclerUI, cyclerData) { Each class could use a comment saying what its purpose is. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:7: * Enum for different capture tab states, showing variously a page for I think I'd move the descriptions of each state into comments before each enum value. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:7: * Enum for different capture tab states, showing variously a page for s/capture/playback/? http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:75: this.yesCaptures_.hidden = true; Did you forget to hide playbackDetails_ here? You should probably move the "playbackTab_.hidden=false" and "3_tabs.hidden=true" code into this.enable, so that you don't have to duplicate it in each of the 3 enable functions. It might be even better to write a little class to manage tabs with optional labels, but this app may not get enough re-use out of it to justify it. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:137: this.enableShowPlayback_(); What happens if they select the 'Choose a capture' option?
Back to you, Jeffrey. Thanks for the comments. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/capture_tab.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/capture_tab.js:72: this.playbackTab_.updatePlaybackChoices_(); On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > It seems odd to update this here without showing the tab. Should capture > completion automatically show the playback tab? Should PlaybackTab.enable() > automatically update the choices? This is an asynchronous return, and it may be somewhat delayed since it requires a full subbrowser execution. It's possible the user will shift to the playback tab while waiting the return. Safest bet is to do it this way, I think, but if you want me to change it anyway, lemme know. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler.html (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:1: <!doctype html5> On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > Isn't this supposed to be "<!doctype html>"? > http://developers.whatwg.org/syntax.html#the-doctype Thot I needed the HTML5 to support the spinner, but evidently not. Fixed. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:38: class='name-combo'/> On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > Don't use two class attributes. Done. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:75: <input id='playback-extension' type='text' /> On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > This should be a file control, right? Or can those not select directories? > > I don't see any use of the playback-browse button. How do you plan to use it? These are for a later CL. Trying to do this in phases since we've been asked to get feedback incrementally. The plan would be to relay a directory of an unpacked extension, yes, which would in turn be the argument of a load-extension commandline argument in the subbrowser http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler_data.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_data.js:5: var CyclerData = function () { On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > I assume you plan to save this object to some sort of persistent storage? Not the object, but the FileSystem it will use will persist, yes. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_data.js:28: * Delete capture |name| from the local FileSystem, and update the On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > Please mark the actual deletion as a TODO. Done, here and on the other two TODO methods as well. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:12: * Enum for different UI states, either capture or playback. On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > You can omit "capture or playback" since those are the names of the enum values. Done. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:34: Object.defineProperty(this, 'currentCaptureName', On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > Why use defineProperty instead of simple assignment? Seemed it was widely used in other webui JS, and also just good general OO discipline. I suppose I could remove it and put it back only if some housekeeping turns out to be needed. IIUC, the syntax of using the property won't change if I do so. Done. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:38: this.playbackTab_ = new PlaybackTab(this, this.cyclerData_); On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > This overwrites an existing variable. Which did you want to keep? Oops :). Thanks. (Ditto for captureTab, of course.) Done http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:46: if (this.enableState != EnableState_.capture) { On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > Where does enableState get initialized? I guess this works because it's > initially 'undefined', which is != to everything? I'm not sure that's good > style. Yes, that was my intent. Wasn't sure if it was good JS style or not. People tend to just make up properties as they like in this language :). I'll add a default setting to null. I don't want to set it to EnableState_.capture, since that should go along with the housekeeping that matches it, and will happen in the call at the bottom. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:50: this.captureTab_.enable(); On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > Arguably, it seems like CaptureTab.enable should imply marking the label as > selected. It also would have to know to deselect all the other labels. Global knowledge of what tab labels there are, and which need selection and deselection, seemed the realm of the overarching UI class. But now I realize I could simply do label deselection in the disable methods. So, fixed. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/manifest.json (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/manifest.json:12: "128": "cycler_icon.png", On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > I'd probably include the _128 in this icon's name. Done. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:5: var PlaybackTab = function (cyclerUI, cyclerData) { On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > Each class could use a comment saying what its purpose is. Done. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:7: * Enum for different capture tab states, showing variously a page for On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > I think I'd move the descriptions of each state into comments before each enum > value. Done. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:7: * Enum for different capture tab states, showing variously a page for On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > I think I'd move the descriptions of each state into comments before each enum > value. Done. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:75: this.yesCaptures_.hidden = true; On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > Did you forget to hide playbackDetails_ here? > Sort of. I forgot to nest playbackDetails under yesCaptures in the HTML, which would make its setting moot when yesCaptures is hidden. Fixed that. > You should probably move the "playbackTab_.hidden=false" and > "3_tabs.hidden=true" code into this.enable, so that you don't have to duplicate > it in each of the 3 enable functions. These methods, at least some, are called from other methods than enable, and thus need to stand alone wrt hiding/showing components. > > It might be even better to write a little class to manage tabs with optional > labels, but this app may not get enough re-use out of it to justify it. I had this before, actually, in that enum :). (I'm a Java programmer, so I expect my enums to work for a living.) But I had to agree with you guys that it was too cute for its own good. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:137: this.enableShowPlayback_(); On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > What happens if they select the 'Choose a capture' option? Nothing. It's disabled in the HTML.
I'll go through your other changes now, but I wanted to give you a chance to start on the rest of my previous comments. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... File chrome/common/extensions/api/experimental_record.json (right): http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:27: "description": "Unique name of the capture.", On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > On 2012/08/10 00:33:13, clintstaley wrote: > > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > > How should extensions allocate these names? What happens if the name > collides > > > with some other capture? Can two different extensions use the same name? > > > > These are capture names, and their uniqueness will be enforced once I add the > > FileSystem storage on the JS side in the next revision. > > What do you mean by "enforced"? That is, what's the consequence of violating the > rule? If an extension calls captureURLs twice with the same captureName, what's > the effect? > > > Not sure what you mean > > by allowing two extensions to use the same name ? > > I'd like you to describe the scope of the uniqueness. If two different > extensions both pass "cat" as the captureName argument to captureURLs(), what > happens? What about the same extension on two different computers that are > sync'ed to the same google account? What about the same extension in two > different sessions on the same computer+profile? Ping. Remember to check for comment replies on more than the most recent patch.
http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/capture_tab.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/capture_tab.js:72: this.playbackTab_.updatePlaybackChoices_(); On 2012/08/15 23:54:20, clintstaley wrote: > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > It seems odd to update this here without showing the tab. Should capture > > completion automatically show the playback tab? Should PlaybackTab.enable() > > automatically update the choices? > > This is an asynchronous return, and it may be somewhat delayed since it requires > a full subbrowser execution. It's possible the user will shift to the playback > tab while waiting the return. Safest bet is to do it this way, I think, but if > you want me to change it anyway, lemme know. Ah, yes. I hadn't thought of the user switching tabs during the run. What happens if they do that and start clicking around, say, starting other playbacks, while the current capture is running? Maybe it'd make sense to disable the playback tab label while the capture is running? Or overlay that tab with a "Capture is running" spinner? http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler.html (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:1: <!doctype html5> On 2012/08/15 23:54:20, clintstaley wrote: > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > Isn't this supposed to be "<!doctype html>"? > > http://developers.whatwg.org/syntax.html#the-doctype > > Thot I needed the HTML5 to support the spinner, but evidently not. Fixed. <!doctype html> actually asks for HTML5. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:75: <input id='playback-extension' type='text' /> On 2012/08/15 23:54:20, clintstaley wrote: > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > This should be a file control, right? Or can those not select directories? > > > > I don't see any use of the playback-browse button. How do you plan to use it? > > These are for a later CL. Trying to do this in phases since we've been asked to > get feedback incrementally. The plan would be to relay a directory of an > unpacked extension, yes, which would in turn be the argument of a load-extension > commandline argument in the subbrowser Delaying the implementation sounds good to me. I realized that, even if a file input doesn't support selecting directories, you can have the user select the manifest.json file, and use getParent() to get the right DirectoryEntry to pass to the playback API. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler_data.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_data.js:5: var CyclerData = function () { On 2012/08/15 23:54:20, clintstaley wrote: > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > I assume you plan to save this object to some sort of persistent storage? > > Not the object, but the FileSystem it will use will persist, yes. Ah, so it'll do the equivalent of an `ls` to populate currentCaptures_ on construction? That makes sense; please add a TODO. (It's so weird to me to have a private file system for each extension's use where you don't have to find a directory structure that won't collide with other users. <attempts to fold brain into matching arrangement>) http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:34: Object.defineProperty(this, 'currentCaptureName', On 2012/08/15 23:54:20, clintstaley wrote: > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > Why use defineProperty instead of simple assignment? > > Seemed it was widely used in other webui JS, and also just good general OO > discipline. I suppose I could remove it and put it back only if some > housekeeping turns out to be needed. IIUC, the syntax of using the property > won't change if I do so. > > Done. I think the good OO practice is to write getters and setters for properties, or to use defineProperty to, say, restrict the writability or deletability of a field. In Python, where @property acts similarly to Object.defineProperty, it's better practice to leave user-accessible fields directly accessible until you need to restrict them or modify their behavior, and only then write a full property. I assume the guidelines are the same in JS. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:75: this.yesCaptures_.hidden = true; On 2012/08/15 23:54:20, clintstaley wrote: > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > Did you forget to hide playbackDetails_ here? > > > > Sort of. I forgot to nest playbackDetails under yesCaptures in the HTML, which > would make its setting moot when yesCaptures is hidden. Fixed that. > > > You should probably move the "playbackTab_.hidden=false" and > > "3_tabs.hidden=true" code into this.enable, so that you don't have to > duplicate > > it in each of the 3 enable functions. > > These methods, at least some, are called from other methods than enable, and > thus need to stand alone wrt hiding/showing components. Ok. > > It might be even better to write a little class to manage tabs with optional > > labels, but this app may not get enough re-use out of it to justify it. > > I had this before, actually, in that enum :). (I'm a Java programmer, so I > expect my enums to work for a living.) But I had to agree with you guys that it > was too cute for its own good. Embedding the tab handling into each state enum wasn't what I was suggesting. It would be possible to write a generic tab-handling class that's not tied to anything concrete in the UI, and then instantiate it multiple times in your app. But, like I said, with only 2 tabbed displays in this app, the abstraction is probably not worth it. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:137: this.enableShowPlayback_(); On 2012/08/15 23:54:20, clintstaley wrote: > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > What happens if they select the 'Choose a capture' option? > > Nothing. It's disabled in the HTML. Hm ... In updatePlaybackChoices_(), I see you clearing out the HTML's disabled option, and adding a 'Choose a capture' option that's _not_ disabled. http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler.html (right): http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:55: <div id='yes-captures'> Nit: "yes-captures" doesn't really mean the opposite of "no-captures", and it keeps making me double-take. Maybe "captures-exist" or "have-captures" or "capture-list" or "capture-selection"? http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:65: </textarea> Nit: </textarea> should be indented to line up with <textarea>. http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js (right): http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:39: * Current state of tab enables. Try not to just restate the name of a variable in its comment. In this case, "Stores which tab is enabled. Takes values from EnableState_." would be better. It's unfortunate that @type doesn't take the name of an enum as a possible argument. http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js (right): http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:16: * Enum for different capture tab states. s/capture/playback/?
Thanks again, Jeffrey. Here are followups on your comments today, plus those I overlooked yesterday. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... File chrome/common/extensions/api/experimental_record.json (right): http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:27: "description": "Unique name of the capture.", On 2012/08/16 18:31:51, Jeffrey Yasskin wrote: > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > On 2012/08/10 00:33:13, clintstaley wrote: > > > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > > > How should extensions allocate these names? What happens if the name > > collides > > > > with some other capture? Can two different extensions use the same name? > > > > > > These are capture names, and their uniqueness will be enforced once I add > the > > > FileSystem storage on the JS side in the next revision. > > > > What do you mean by "enforced"? That is, what's the consequence of violating > the > > rule? If an extension calls captureURLs twice with the same captureName, > what's > > the effect? > > > > > Not sure what you mean > > > by allowing two extensions to use the same name ? > > > > I'd like you to describe the scope of the uniqueness. If two different > > extensions both pass "cat" as the captureName argument to captureURLs(), what > > happens? What about the same extension on two different computers that are > > sync'ed to the same google account? What about the same extension in two > > different sessions on the same computer+profile? > > Ping. Remember to check for comment replies on more than the most recent patch. Apologies for that. I've been looking at the CL's themselves, and I should probably be walking down the summary Emails point by point instead. In any event, I think these parameters are destined for the ash heap based on Aaron's switch back to transferring zip files with cache content, and storing them on the extension side. The only reason for the unique name originally was to identify what FileSystem dir from which to draw cache data, but if that's coming across now as anonymous zip content, I'll be dropping these from the next rev. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:64: "description": "Unique name of capture. Use to determine cache." On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > On 2012/08/10 00:33:13, clintstaley wrote: > > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > > How is it used to determine the cache? Presumably you mean that this has to > > > match the name passed to a previous captureURLs call, but in that case, why > > not > > > have captureURLs "return" an object representing the capture, and pass that > > into > > > replayURLs instead of making the user track strings? > > > > Such an approach was considered (returning a zip of the cache dir, to be > stored > > on the JS side in FileSystem storage and sent back on playback). But, Aaron > > preferred an approach where we would write to and from FileSystem storage on > the > > C++ side, so all the JS side needs is to track the names of captures, which > are > > also the names of the FileSystem directories into which the capture state will > > be saved. > > Can the extension get at the storage directory using any other FileSystem APIs? > If so, maybe the extension should just pass a DirectoryEntry to this function? > At the very least, the comment needs to say where the name comes from, and how > it's used to determine the cache. > > On the other hand, I think Aaron said this function should just take a zip file > now. Right, which makes the other stuff moot. I guess I'd have been surprised anyway if an extension that is essentially "third party" in design (no webui, etc) could have such access, but I don't know this stuff as well as you. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:85: "description": "Time in milliseconds to complete all runs." On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > On 2012/08/10 00:33:13, clintstaley wrote: > > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > > I saw there's a new performance-measuring API that uses a double number of > > > milliseconds to capture microseconds: > > > http://gent.ilcore.com/2012/06/better-timer-for-javascript.html. Have you > > > considered doing the same thing here? > > > > Given the size of the times we're getting, not sure that resolution is needed. > > > > It almost certainly won't be needed for the next couple years, but it costs you > very little to return a double from this interface instead of an int, and it > makes sure you won't need to extend the interface in the future, if people do > want more precise results. Done. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:90: "description": "Full multiline dump of output stats." On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > In what format? Here again, is something that will be refined in later version (sorry to keep saying that). Aaron suggests nice charts of performance data, and at present we have a big ugly text dump. I'll describe it here some more, to be complete, but it's going to change. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:96: "description": "List of errors during replay." On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > On 2012/08/10 00:33:13, clintstaley wrote: > > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > > What do errors look like? What kinds of errors should be expected? > > > > I can show you some examples. But I think they'd be a bit lengthy to put into > a > > JSON description field. Should I be making these description fields longer?? > > > > Yes, I think so. Remember that this is all programmers have when they're trying > to figure out how to use the API, and you don't want them to have to > guess-and-check, since they're likely to draw the wrong conclusions from that. Especially once the capture name is removed, there won't be much to report aside from unexpected faults, but I did add that to the description, and will pay more attention to these descriptions going forward. Hadn't realized they were used that much. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/capture_tab.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/capture_tab.js:72: this.playbackTab_.updatePlaybackChoices_(); On 2012/08/16 20:19:31, Jeffrey Yasskin wrote: > On 2012/08/15 23:54:20, clintstaley wrote: > > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > > It seems odd to update this here without showing the tab. Should capture > > > completion automatically show the playback tab? Should PlaybackTab.enable() > > > automatically update the choices? > > > > This is an asynchronous return, and it may be somewhat delayed since it > requires > > a full subbrowser execution. It's possible the user will shift to the > playback > > tab while waiting the return. Safest bet is to do it this way, I think, but > if > > you want me to change it anyway, lemme know. > > Ah, yes. I hadn't thought of the user switching tabs during the run. What > happens if they do that and start clicking around, say, starting other > playbacks, while the current capture is running? Maybe it'd make sense to > disable the playback tab label while the capture is running? Or overlay that tab > with a "Capture is running" spinner? A later CL will include the ability to cancel an ongoing capture, with a modal dialog the user. Don't want to add it just yet, until supporting logic on the C++ end is done. But, thinking about that, it does mean I can move that update choices to the playback tab enable, which is more conventional design. Going straight to playback isn't in the current spec, and might be irritating to a user trying to create several captures in sequence. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler_data.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_data.js:5: var CyclerData = function () { On 2012/08/16 20:19:31, Jeffrey Yasskin wrote: > On 2012/08/15 23:54:20, clintstaley wrote: > > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > > I assume you plan to save this object to some sort of persistent storage? > > > > Not the object, but the FileSystem it will use will persist, yes. > > Ah, so it'll do the equivalent of an `ls` to populate currentCaptures_ on > construction? That makes sense; please add a TODO. > > (It's so weird to me to have a private file system for each extension's use > where you don't have to find a directory structure that won't collide with other > users. <attempts to fold brain into matching arrangement>) Yeah, I found it took a little getting used to too :) TODO added on getCaptures. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:34: Object.defineProperty(this, 'currentCaptureName', On 2012/08/16 20:19:31, Jeffrey Yasskin wrote: > On 2012/08/15 23:54:20, clintstaley wrote: > > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > > Why use defineProperty instead of simple assignment? > > > > Seemed it was widely used in other webui JS, and also just good general OO > > discipline. I suppose I could remove it and put it back only if some > > housekeeping turns out to be needed. IIUC, the syntax of using the property > > won't change if I do so. > > > > Done. > > I think the good OO practice is to write getters and setters for properties, or > to use defineProperty to, say, restrict the writability or deletability of a > field. In Python, where @property acts similarly to Object.defineProperty, it's > better practice to leave user-accessible fields directly accessible until you > need to restrict them or modify their behavior, and only then write a full > property. I assume the guidelines are the same in JS. This way in C# too. Wish they'd add properties to Java. So much nicer than all the getter/setter noise. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:137: this.enableShowPlayback_(); On 2012/08/16 20:19:31, Jeffrey Yasskin wrote: > On 2012/08/15 23:54:20, clintstaley wrote: > > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > > What happens if they select the 'Choose a capture' option? > > > > Nothing. It's disabled in the HTML. > > Hm ... In updatePlaybackChoices_(), I see you clearing out the HTML's disabled > option, and adding a 'Choose a capture' option that's _not_ disabled. Hmm. Right you are. It wasn't making any choices when selected, but it was selectable. Fixed. http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler.html (right): http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:55: <div id='yes-captures'> On 2012/08/16 20:19:31, Jeffrey Yasskin wrote: > Nit: "yes-captures" doesn't really mean the opposite of "no-captures", and it > keeps making me double-take. Maybe "captures-exist" or "have-captures" or > "capture-list" or "capture-selection"? Done. http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:55: <div id='yes-captures'> On 2012/08/16 20:19:31, Jeffrey Yasskin wrote: > Nit: "yes-captures" doesn't really mean the opposite of "no-captures", and it > keeps making me double-take. Maybe "captures-exist" or "have-captures" or > "capture-list" or "capture-selection"? Done. http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler.html:65: </textarea> On 2012/08/16 20:19:31, Jeffrey Yasskin wrote: > Nit: </textarea> should be indented to line up with <textarea>. Done. http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js (right): http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js:39: * Current state of tab enables. On 2012/08/16 20:19:31, Jeffrey Yasskin wrote: > Try not to just restate the name of a variable in its comment. In this case, > "Stores which tab is enabled. Takes values from EnableState_." would be better. > It's unfortunate that @type doesn't take the name of an enum as a possible > argument. Done. http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js (right): http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:16: * Enum for different capture tab states. On 2012/08/16 20:19:31, Jeffrey Yasskin wrote: > s/capture/playback/? Done. http://codereview.chromium.org/10832191/diff/3004/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js:16: * Enum for different capture tab states. On 2012/08/16 20:19:31, Jeffrey Yasskin wrote: > s/capture/playback/? Done.
LGTM with the run_time_ms_ change. Thanks! http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... File chrome/common/extensions/api/experimental_record.json (right): http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:27: "description": "Unique name of the capture.", On 2012/08/17 04:12:24, clintstaley wrote: > On 2012/08/16 18:31:51, Jeffrey Yasskin wrote: > > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > > On 2012/08/10 00:33:13, clintstaley wrote: > > > > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > > > > How should extensions allocate these names? What happens if the name > > > collides > > > > > with some other capture? Can two different extensions use the same name? > > > > > > > > These are capture names, and their uniqueness will be enforced once I add > > the > > > > FileSystem storage on the JS side in the next revision. > > > > > > What do you mean by "enforced"? That is, what's the consequence of violating > > the > > > rule? If an extension calls captureURLs twice with the same captureName, > > what's > > > the effect? > > > > > > > Not sure what you mean > > > > by allowing two extensions to use the same name ? > > > > > > I'd like you to describe the scope of the uniqueness. If two different > > > extensions both pass "cat" as the captureName argument to captureURLs(), > what > > > happens? What about the same extension on two different computers that are > > > sync'ed to the same google account? What about the same extension in two > > > different sessions on the same computer+profile? > > > > Ping. Remember to check for comment replies on more than the most recent > patch. > > Apologies for that. I've been looking at the CL's themselves, and I should > probably be walking down the summary Emails point by point instead. > > In any event, I think these parameters are destined for the ash heap based on > Aaron's switch back to transferring zip files with cache content, and storing > them on the extension side. The only reason for the unique name originally was > to identify what FileSystem dir from which to draw cache data, but if that's > coming across now as anonymous zip content, I'll be dropping these from the next > rev. SGTM. http://codereview.chromium.org/10832191/diff/1001/chrome/common/extensions/ap... chrome/common/extensions/api/experimental_record.json:90: "description": "Full multiline dump of output stats." On 2012/08/17 04:12:24, clintstaley wrote: > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > In what format? > > Here again, is something that will be refined in later version (sorry to keep > saying that). Aaron suggests nice charts of performance data, and at present we > have a big ugly text dump. I'll describe it here some more, to be complete, but > it's going to change. That's fine. Having a precise format all along will remind us to keep the format precise for the final documentation. FWIW, you're likely to wind up with an object-based stats format instead of a string in the long run. http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... File chrome/common/extensions/docs/examples/apps/cycler/capture_tab.js (right): http://codereview.chromium.org/10832191/diff/3002/chrome/common/extensions/do... chrome/common/extensions/docs/examples/apps/cycler/capture_tab.js:72: this.playbackTab_.updatePlaybackChoices_(); On 2012/08/17 04:12:25, clintstaley wrote: > On 2012/08/16 20:19:31, Jeffrey Yasskin wrote: > > On 2012/08/15 23:54:20, clintstaley wrote: > > > On 2012/08/15 01:03:59, Jeffrey Yasskin wrote: > > > > It seems odd to update this here without showing the tab. Should capture > > > > completion automatically show the playback tab? Should > PlaybackTab.enable() > > > > automatically update the choices? > > > > > > This is an asynchronous return, and it may be somewhat delayed since it > > requires > > > a full subbrowser execution. It's possible the user will shift to the > > playback > > > tab while waiting the return. Safest bet is to do it this way, I think, but > > if > > > you want me to change it anyway, lemme know. > > > > Ah, yes. I hadn't thought of the user switching tabs during the run. What > > happens if they do that and start clicking around, say, starting other > > playbacks, while the current capture is running? Maybe it'd make sense to > > disable the playback tab label while the capture is running? Or overlay that > tab > > with a "Capture is running" spinner? > > A later CL will include the ability to cancel an ongoing capture, with a modal > dialog the user. Don't want to add it just yet, until supporting logic on the > C++ end is done. > > But, thinking about that, it does mean I can move that update choices to the > playback tab enable, which is more conventional design. > > Going straight to playback isn't in the current spec, and might be irritating to > a user trying to create several captures in sequence. You may want to make the cyclerData observable (yay Model pattern), and have the playback tab observe it, so it can do something appropriate when the data changes asynchronously. Not in this CL though. http://codereview.chromium.org/10832191/diff/10013/chrome/browser/extensions/... File chrome/browser/extensions/api/record/record_api.cc (right): http://codereview.chromium.org/10832191/diff/10013/chrome/browser/extensions/... chrome/browser/extensions/api/record/record_api.cc:246: run_time_ms_ = (base::Time::NowFromSystemTime() - timer_).InMilliseconds(); Switch this to InMillisecondsF and change run_time_ms_ to a double, so users don't get an artificially-truncated value. http://codereview.chromium.org/10832191/diff/10013/chrome/browser/extensions/... chrome/browser/extensions/api/record/record_api.cc:250: results_ = record::ReplayURLs::Results::Create((double)run_time_ms_, stats_, In C++, ints implicitly convert to doubles, so you don't need the cast.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10832191/9006
Failed to request the patch to try. Please note that binary filesare still unsupported at the moment, this is being worked on. Thanks for your patience. HTTP Error 404: Not Found
Thanks. run_time_ms is changed now https://chromiumcodereview.appspot.com/10832191/diff/1001/chrome/common/exten... File chrome/common/extensions/api/experimental_record.json (right): https://chromiumcodereview.appspot.com/10832191/diff/1001/chrome/common/exten... chrome/common/extensions/api/experimental_record.json:90: "description": "Full multiline dump of output stats." On 2012/08/17 19:32:14, Jeffrey Yasskin wrote: > On 2012/08/17 04:12:24, clintstaley wrote: > > On 2012/08/09 19:27:04, Jeffrey Yasskin wrote: > > > In what format? > > > > Here again, is something that will be refined in later version (sorry to keep > > saying that). Aaron suggests nice charts of performance data, and at present > we > > have a big ugly text dump. I'll describe it here some more, to be complete, > but > > it's going to change. > > That's fine. Having a precise format all along will remind us to keep the format > precise for the final documentation. FWIW, you're likely to wind up with an > object-based stats format instead of a string in the long run. I wouldn't bet against you. That's almost certainly what we'll end up with. https://chromiumcodereview.appspot.com/10832191/diff/10013/chrome/browser/ext... File chrome/browser/extensions/api/record/record_api.cc (right): https://chromiumcodereview.appspot.com/10832191/diff/10013/chrome/browser/ext... chrome/browser/extensions/api/record/record_api.cc:246: run_time_ms_ = (base::Time::NowFromSystemTime() - timer_).InMilliseconds(); On 2012/08/17 19:32:14, Jeffrey Yasskin wrote: > Switch this to InMillisecondsF and change run_time_ms_ to a double, so users > don't get an artificially-truncated value. Done. https://chromiumcodereview.appspot.com/10832191/diff/10013/chrome/browser/ext... chrome/browser/extensions/api/record/record_api.cc:250: results_ = record::ReplayURLs::Results::Create((double)run_time_ms_, stats_, On 2012/08/17 19:32:14, Jeffrey Yasskin wrote: > In C++, ints implicitly convert to doubles, so you don't need the cast. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10832191/9006
Failed to request the patch to try. Please note that binary filesare still unsupported at the moment, this is being worked on. Thanks for your patience. HTTP Error 404: Not Found
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10832191/11009
Failed to request the patch to try. Please note that binary filesare still unsupported at the moment, this is being worked on. Thanks for your patience. HTTP Error 404: Not Found
Aaron, can I also get your LGTM. yyasskin doesn't appear to be on the necessary OWNERS list.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10832191/4010
Presubmit check for 10832191-4010 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** This change modifies the extension docs but the generated docs have not been updated properly. See chrome/common/extensions/docs/README.txt for more info. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/capture_tab.js have not been re-zipped. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/cycler.css have not been re-zipped. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/cycler.html have not been re-zipped. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/cycler_data.js have not been re-zipped. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js have not been re-zipped. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/manifest.json have not been re-zipped. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js have not been re-zipped. First build DumpRenderTree, then update the docs by running: chrome/common/extensions/docs/build/build.py --page-name=<apiName> Presubmit checks took 1.4s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10832191/9008
Presubmit check for 10832191-9008 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** This change modifies the extension docs but the generated docs have not been updated properly. See chrome/common/extensions/docs/README.txt for more info. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/capture_tab.js have not been re-zipped. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/cycler.css have not been re-zipped. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/cycler.html have not been re-zipped. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/cycler_data.js have not been re-zipped. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/cycler_ui.js have not been re-zipped. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/manifest.json have not been re-zipped. - Changes to sample chrome/common/extensions/docs/examples/apps/cycler/playback_tab.js have not been re-zipped. First build DumpRenderTree, then update the docs by running: chrome/common/extensions/docs/build/build.py --page-name=<apiName> Presubmit checks took 1.6s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10832191/3037
Failed to apply patch for chrome/common/extensions/docs/samples.json: While running patch -p1 --forward --force; patching file chrome/common/extensions/docs/samples.json Hunk #1 FAILED at 185. Hunk #2 succeeded at 1064 (offset 2 lines). 1 out of 2 hunks FAILED -- saving rejects to file chrome/common/extensions/docs/samples.json.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10832191/40
Try job failure for 10832191-40 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10832191/21001
Can't process patch for file chrome/common/extensions/docs/examples/apps/cycler/cycler_icon.png. Binary file support is temporarilly disabled due to a bug. Please commit blindly the binary files first then commit the source change as a separate CL. Sorry for the annoyance.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clintstaley@chromium.org/10832191/26001
Change committed as 162059 |
