Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2038963002:
[Offline Pages] Link the internals page with the offline model and request (Closed)
|
Created:
4 years, 6 months ago by chili Modified:
4 years, 6 months ago CC:
arv+watch_chromium.org, asvitkine+watch_chromium.org, chromium-reviews, groby-ooo-7-16 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Link the internals page with the offline model and request
coordinators
- Adds functionality to 'dump' all content into a csv file for download.
- Adds adds the resource files to closure
BUG=609570
DIFF_BASE=2003883002
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/27d6f0a481a7f73b7dd60f894000c8d22f735332
Cr-Commit-Position: refs/heads/master@{#399818}
Patch Set 1 #Patch Set 2 : try dependency fix #Patch Set 3 : try to fix patch dependency #2 #
Total comments: 25
Patch Set 4 : #
Total comments: 3
Patch Set 5 : #
Total comments: 78
Patch Set 6 : #
Total comments: 16
Patch Set 7 : More code review comment fixes #
Total comments: 2
Patch Set 8 : More code review comment fixes #Patch Set 9 : More code review comment fixes #
Total comments: 7
Patch Set 10 : Download JSON rather than CSV #
Total comments: 29
Patch Set 11 : changing from csv to json #Patch Set 12 : changing from csv to json #
Total comments: 6
Patch Set 13 : small fixes #
Dependent Patchsets: Messages
Total messages: 58 (13 generated)
Description was changed from ========== [Offline Pages] Link the internals page with the offline model and request coordinators Also adds functionality to 'dump' all content into a csv file for download. BUG=609570 BASE=2003883002 ========== to ========== [Offline Pages] Link the internals page with the offline model and request coordinators Also adds functionality to 'dump' all content into a csv file for download. BUG=609570 BASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Offline Pages] Link the internals page with the offline model and request coordinators Also adds functionality to 'dump' all content into a csv file for download. BUG=609570 BASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Offline Pages] Link the internals page with the offline model and request coordinators Also adds functionality to 'dump' all content into a csv file for download. BUG=609570 BASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
chili@chromium.org changed reviewers: + dewittj@chromium.org
Hopefully the base CL won't change much. This adds in hooks for the actual offline data. Not sure how to define a diffbase :/
Description was changed from ========== [Offline Pages] Link the internals page with the offline model and request coordinators Also adds functionality to 'dump' all content into a csv file for download. BUG=609570 BASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Offline Pages] Link the internals page with the offline model and request coordinators Also adds functionality to 'dump' all content into a csv file for download. BUG=609570 DIFFBASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Offline Pages] Link the internals page with the offline model and request coordinators Also adds functionality to 'dump' all content into a csv file for download. BUG=609570 DIFFBASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Offline Pages] Link the internals page with the offline model and request coordinators Also adds functionality to 'dump' all content into a csv file for download. BUG=609570 DIFF_BASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
If you set the upstream branch in your local git repo to another branch that has an issue on codereview, it will do the diffbase for you. git checkout branch1 origin/master ... work ... git commit -am 'done.' git cl upload git checkout branch2 git branch --set-upstream-to=branch1 ... work ... git commit -am 'again I am done.' git cl upload does the trick.
On 2016/06/03 22:14:33, dewittj wrote: > If you set the upstream branch in your local git repo to another branch that has > an issue on codereview, it will do the diffbase for you. > > git checkout branch1 origin/master > ... work ... > git commit -am 'done.' > git cl upload > git checkout branch2 > git branch --set-upstream-to=branch1 > ... work ... > git commit -am 'again I am done.' > git cl upload > > does the trick. Done :)
nice. https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.html:41: <div id="random-info"></div> Is it really random? :) https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:34: var currentStoredPagesData = null; needs /** @type {...} */ https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:41: function clearTable(tableId) { nit: make this take an element, then pass it in; this way you are searching the document by ID only once in each function that clears a table. https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:47: * @param {!Array<OfflinePageItem} data An array object representing Missing ">" https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:138: * @param {string} strObj The obj to add quotes around. @return {string} the escaped string https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:140: function addQuotes(strObj) { need to escape backslashes and quotes in the incoming string here. https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:186: window.open(uriContent, 'dump.csv'); Does this open a window, or does it cause a download? https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:67: std::string GetStringFromDeletePageResult( Please define out of line. This might be better defined in offline_page_types.h so that people adding new statuses will be inclined to add a new one. Same with SavePageStatus https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:113: offline_pages::DeletePageResult::SUCCESS)); Please add a comment that this is here because ClearAll does not call back with a status code. https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:128: offline_ids.push_back(atoll(value.c_str())); not sure if there is some function in Chromium to use besides atoll. strtoll is probably preferred (since you can set base there)... There is also ParseInt64, but that's in net::, so not really likely to be used for general purpose parsing. https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:154: js_page_object->SetString("onlineUrl", page.url.spec()); I wish we didn't have so many hard-coded strings around but I don't know how to change that. https://codereview.chromium.org/2038963002/diff/40001/chrome/common/url_const... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2038963002/diff/40001/chrome/common/url_const... chrome/common/url_constants.cc:141: const char kChromeUIOfflineInternalsURL[] = "chrome://offline-internals"; Is this a merge error? I would think this would already be here.
Description was changed from ========== [Offline Pages] Link the internals page with the offline model and request coordinators Also adds functionality to 'dump' all content into a csv file for download. BUG=609570 DIFF_BASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Offline Pages] Link the internals page with the offline model and request coordinators - Adds functionality to 'dump' all content into a csv file for download. - Adds adds the resource files to closure BUG=609570 DIFF_BASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
chili@chromium.org changed reviewers: + dbeam@chromium.org, dpapad@chromium.org
https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.html:41: <div id="random-info"></div> On 2016/06/03 23:12:05, dewittj wrote: > Is it really random? :) Done. https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:34: var currentStoredPagesData = null; On 2016/06/03 23:12:05, dewittj wrote: > needs /** @type {...} */ Done. https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:41: function clearTable(tableId) { On 2016/06/03 23:12:05, dewittj wrote: > nit: make this take an element, then pass it in; this way you are searching the > document by ID only once in each function that clears a table. Done. https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:47: * @param {!Array<OfflinePageItem} data An array object representing On 2016/06/03 23:12:06, dewittj wrote: > Missing ">" Done. https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:138: * @param {string} strObj The obj to add quotes around. On 2016/06/03 23:12:05, dewittj wrote: > @return {string} the escaped string Done. https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:140: function addQuotes(strObj) { On 2016/06/03 23:12:06, dewittj wrote: > need to escape backslashes and quotes in the incoming string here. backslashes don't need to be escaped. Added escaping for quotes https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:186: window.open(uriContent, 'dump.csv'); On 2016/06/03 23:12:06, dewittj wrote: > Does this open a window, or does it cause a download? This will open a window that'll ask you to download. Unfortunately, while it's named 'dump.csv', the file downloaded will be download.csv. According to an old stackoverflow thread, chrome seemed to have lost ability to obey default file download names clicking 'open' on the download will display csv in tabular format https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:67: std::string GetStringFromDeletePageResult( On 2016/06/03 23:12:06, dewittj wrote: > Please define out of line. > > This might be better defined in offline_page_types.h so that people adding new > statuses will be inclined to add a new one. Same with SavePageStatus I was hesitating between putting this here vs offline_page_types.h. Nowhere else seems to need these statuses in string form so it doesn't seem to quite fit in offline_page_types. I figured if they needed to see the status in the internals page, they'll see 'Unknown" and try to update. https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:113: offline_pages::DeletePageResult::SUCCESS)); On 2016/06/03 23:12:06, dewittj wrote: > Please add a comment that this is here because ClearAll does not call back with > a status code. Done. https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:128: offline_ids.push_back(atoll(value.c_str())); On 2016/06/03 23:12:06, dewittj wrote: > not sure if there is some function in Chromium to use besides atoll. strtoll > is probably preferred (since you can set base there)... > > There is also ParseInt64, but that's in net::, so not really likely to be used > for general purpose parsing. you can set base in strtoll, but I didn't think the extra parameters were necessary... https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:154: js_page_object->SetString("onlineUrl", page.url.spec()); On 2016/06/03 23:12:06, dewittj wrote: > I wish we didn't have so many hard-coded strings around but I don't know how to > change that. I do too, but I chose to hard-code them here rather than setting constants because - they're only used once in this cc file, and the javascript can't reference these constants anyway. When I talked with dpapad@, he mentioned he's going for a mojom (mojo-like) approach that would be able to autogenerate these things given a common schema. We might be able to eliminate this kind of hard coding altogether, though the timing may be months in the future. https://codereview.chromium.org/2038963002/diff/40001/chrome/common/url_const... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2038963002/diff/40001/chrome/common/url_const... chrome/common/url_constants.cc:141: const char kChromeUIOfflineInternalsURL[] = "chrome://offline-internals"; On 2016/06/03 23:12:06, dewittj wrote: > Is this a merge error? I would think this would already be here. Yes. Merge error
I don't see the new patchset, is it uploaded?
On 2016/06/09 22:37:05, dewittj wrote: > I don't see the new patchset, is it uploaded? Oops. Uploaded now.
https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:15: * }} indent off by 1 \s https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {Array<OfflinePageItem>} */ nit: ?Array<!OfflinePageItems> ! means "not null" ? means "or null" the absence of "?" or "!" means "or null but the reader isn't very sure" ;) https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:42: * @param {!Array<OfflinePageItem>} data An array object representing why did you revert pages to data? https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:42: * @param {!Array<OfflinePageItem>} data An array object representing unless this array can have null in it, !Array<!OfflinePageItem> ^ https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:46: var element = $('stored-pages'); element -> storedPages? https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:83: function fillRequestQueue(data) { same question regarding data vs requests https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:104: currentRequestQueueData = data; currentRequests? https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:137: strObj.replace(/"/g, '""'); // CSV single quotes are encoded as "". this returns a new string and does not work in place (i.e. it currently does nothing) https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:138: return '"' + strObj + '"'; // There can be commas in the string. return '"' + strObj.replace(/"/g, '""') + '"'; https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:184: } is there any particular reason you want to do this in JS instead of in C++? have you seen: components/password_manager/core/browser/export/csv_writer.h maybe this could be shared? also, if you do end up doing this in JS: maybe add a little OO to this code, i.e. make a CSVWriter class that can hide some of these details (i.e. escaping, line joining, etc.)? https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:194: if (checkboxes[i].checked) { no curlies https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:47: void HandleGetRequestQueue(const base::ListValue* args); typically these are all just named HandleExactNameOfChromeSendOrCrSendWithPromise and also document arguments (or that there are no arguments) https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:65: doc comment https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:68: doc comment https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:73: offline_pages::OfflinePageModel* offline_page_model_; where is this initialized originally? https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:73: offline_pages::OfflinePageModel* offline_page_model_; nit: \n https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:89: offline_pages::DeletePageResult value) { nit: I think this implementation would be more future-proof and typesafe: std::string OfflineInternalsUIMessageHandler::DeletePageResultToString( offline_pages::DeletePageResult result) { switch (result) { case SomeResult: return "Some string"; ... } NOTREACHED(); return "Unknown"; } https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:95: else no else after return https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:106: else no else after return https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:112: const base::Value* callback_id; what's the ownership of |callback_id|? is it safe to pass into base::Bind() like this? https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:134: offline_ids.push_back(atoll(value.c_str())); is this different/better than base::StringToInt64()? https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.h... https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:204: if (request_coordinator_ != nullptr) nit: curlies around 2+ line ifs https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:204: if (request_coordinator_ != nullptr) nit: if (request_coordinator_) seems equivalent but shorter https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:234: "getRequestQueueInfo", nit: could this be just "getRequestQueue"? https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:238: "getStoredPagesInfo", could this just be "getStoredPages"?
https://codereview.chromium.org/2038963002/diff/60001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/60001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:42: * @param {!Array<OfflinePageItem>} data An array object representing Can the array hold null/undefined values? If not then the annotation should be as follows (here and elsewhere): @param {!Array<!OfflinePageItem>} https://codereview.chromium.org/2038963002/diff/60001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:87: for (var i = 0; i < data.length; i++) { Nit (optional): You can use forEach instead data.forEach(function(request) { ... cell.textContent = request.onlineUrl; ... }); https://codereview.chromium.org/2038963002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2038963002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:119: #include "chrome/browser/ui/webui/offline_internals_ui.h" Isn't this WebUI page only for Android? https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:28: namespace { Nit: Should this be in offline_pages namespace? Perhaps this would eliminate the need to repeat offline_pages:: multiple times in this file. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:101: const std::string kRequestStatusToString[] = { This constant array is identical with line 91. Should it be moved outside the function and shared between the two functions? https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:113: args->Get(0, &callback_id); CHECK(args->Get(0, &callback_id)); https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:146: Nit: No need for \n here. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:204: if (request_coordinator_ != nullptr) The Promise that JS holds is never fulfilled (resolved or rejected) if this condition is false. Can we handle this case such that no Promise is left indefinitely un-fulfiled. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:205: request_coordinator_->queue()->GetRequests( Nit: Perhaps more readable to use curly braces for this if block, since it spans multiple lines. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:221: weak_ptr_factory_.GetWeakPtr(), callback_id)); Same here, what happens to the Promise?
https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2038963002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:67: std::string GetStringFromDeletePageResult( On 2016/06/09 22:29:16, chili wrote: > On 2016/06/03 23:12:06, dewittj wrote: > > Please define out of line. > > > > This might be better defined in offline_page_types.h so that people adding new > > statuses will be inclined to add a new one. Same with SavePageStatus > > I was hesitating between putting this here vs offline_page_types.h. Nowhere > else seems to need these statuses in string form so it doesn't seem to quite fit > in offline_page_types. I figured if they needed to see the status in the > internals page, they'll see 'Unknown" and try to update. Not everyone is going to necessarily use this page all the time... Could you at least add a comment at DeletePageResult's definition (and SavePageResult) to update the list here when changing the enums? https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {Array<OfflinePageItem>} */ On 2016/06/09 23:35:01, Dan Beam wrote: > nit: ?Array<!OfflinePageItems> > > ! means "not null" > ? means "or null" > > the absence of "?" or "!" means "or null but the reader isn't very sure" ;) I don't think "!" is required for record types (which OfflinePageItem is). https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:134: * @return {string} the escaped string. nit: capitalize the T
https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {Array<OfflinePageItem>} */ On 2016/06/09 23:41:25, dewittj wrote: > On 2016/06/09 23:35:01, Dan Beam wrote: > > nit: ?Array<!OfflinePageItems> > > > > ! means "not null" > > ? means "or null" > > > > the absence of "?" or "!" means "or null but the reader isn't very sure" ;) > > I don't think "!" is required for record types (which OfflinePageItem is). @typedefs are nullable by default
https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {Array<OfflinePageItem>} */ On 2016/06/09 23:50:01, Dan Beam wrote: > On 2016/06/09 23:41:25, dewittj wrote: > > On 2016/06/09 23:35:01, Dan Beam wrote: > > > nit: ?Array<!OfflinePageItems> > > > > > > ! means "not null" > > > ? means "or null" > > > > > > the absence of "?" or "!" means "or null but the reader isn't very sure" ;) > > > > I don't think "!" is required for record types (which OfflinePageItem is). > > @typedefs are nullable by default Can you point me to documentation to that effect? I didn't think @typedef changed the nullability of the represented type. I couldn't find any public documentation about record type nullability, but here's a google-internal link: go/jstype
https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {Array<OfflinePageItem>} */ On 2016/06/10 00:13:49, dewittj wrote: > On 2016/06/09 23:50:01, Dan Beam wrote: > > On 2016/06/09 23:41:25, dewittj wrote: > > > On 2016/06/09 23:35:01, Dan Beam wrote: > > > > nit: ?Array<!OfflinePageItems> > > > > > > > > ! means "not null" > > > > ? means "or null" > > > > > > > > the absence of "?" or "!" means "or null but the reader isn't very sure" > ;) > > > > > > I don't think "!" is required for record types (which OfflinePageItem is). > > > > @typedefs are nullable by default > > Can you point me to documentation to that effect? I didn't think @typedef > changed the nullability of the represented type. > > I couldn't find any public documentation about record type nullability, but > here's a google-internal link: > go/jstype > > > eh, do whatever i care far less about what you do here than finding the doc (if I'm right, not as sure any more) sorry for bringing it up, nullability is pretty lame IMO
https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {Array<OfflinePageItem>} */ FYI, it seems that non-nullable is the default for typedefs, see example below. https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClo... I agree that the nullability concept of the JS compiler is confusing, since some types are nullable by default, and others are not (string, number, boolean). I would prefer having consistent nullability semantics values for all types.
The addition to the closure compiler's gyp triggers a presubmit warning that I need to update the README.chromium file as well. Do you know what exactly I should be doing there? Thanks for the quick responses! https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:15: * }} On 2016/06/09 23:35:01, Dan Beam wrote: > indent off by 1 \s Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {Array<OfflinePageItem>} */ On 2016/06/09 23:35:01, Dan Beam wrote: > nit: ?Array<!OfflinePageItems> > > ! means "not null" > ? means "or null" > > the absence of "?" or "!" means "or null but the reader isn't very sure" ;) Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {Array<OfflinePageItem>} */ On 2016/06/09 23:41:25, dewittj wrote: > On 2016/06/09 23:35:01, Dan Beam wrote: > > nit: ?Array<!OfflinePageItems> > > > > ! means "not null" > > ? means "or null" > > > > the absence of "?" or "!" means "or null but the reader isn't very sure" ;) > > I don't think "!" is required for record types (which OfflinePageItem is). Acknowledged. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {Array<OfflinePageItem>} */ On 2016/06/09 23:50:01, Dan Beam wrote: > On 2016/06/09 23:41:25, dewittj wrote: > > On 2016/06/09 23:35:01, Dan Beam wrote: > > > nit: ?Array<!OfflinePageItems> > > > > > > ! means "not null" > > > ? means "or null" > > > > > > the absence of "?" or "!" means "or null but the reader isn't very sure" ;) > > > > I don't think "!" is required for record types (which OfflinePageItem is). > > @typedefs are nullable by default Acknowledged. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:42: * @param {!Array<OfflinePageItem>} data An array object representing On 2016/06/09 23:35:01, Dan Beam wrote: > unless this array can have null in it, !Array<!OfflinePageItem> > ^ Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:42: * @param {!Array<OfflinePageItem>} data An array object representing On 2016/06/09 23:35:01, Dan Beam wrote: > why did you revert pages to data? I had this CL sitting for almost as long as the previous one. The merge didn't go correctly https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:46: var element = $('stored-pages'); On 2016/06/09 23:35:01, Dan Beam wrote: > element -> storedPages? Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:83: function fillRequestQueue(data) { On 2016/06/09 23:35:01, Dan Beam wrote: > same question regarding data vs requests Same as above... bad merge https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:104: currentRequestQueueData = data; On 2016/06/09 23:35:01, Dan Beam wrote: > currentRequests? Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:134: * @return {string} the escaped string. On 2016/06/09 23:41:25, dewittj wrote: > nit: capitalize the T Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:137: strObj.replace(/"/g, '""'); // CSV single quotes are encoded as "". On 2016/06/09 23:35:01, Dan Beam wrote: > this returns a new string and does not work in place (i.e. it currently does > nothing) oops. fixed. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:138: return '"' + strObj + '"'; // There can be commas in the string. On 2016/06/09 23:35:01, Dan Beam wrote: > return '"' + strObj.replace(/"/g, '""') + '"'; Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:184: } On 2016/06/09 23:35:01, Dan Beam wrote: > is there any particular reason you want to do this in JS instead of in C++? > > have you seen: components/password_manager/core/browser/export/csv_writer.h > > maybe this could be shared? > > also, if you do end up doing this in JS: maybe add a little OO to this code, > i.e. make a CSVWriter class that can hide some of these details (i.e. escaping, > line joining, etc.)? Not knowing too much about webui and how to get downloads to call into C++ with a filepath, I thought to get something working first. Will probably ping you about that part. If alright with you, I'd also like to push creating CSVWriter to the next CL. I'm planning to use the proxy stuff I talked with dpapad@ in the next CL, which is going to involve restructuring this class a bit anyway. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:194: if (checkboxes[i].checked) { On 2016/06/09 23:35:01, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:47: void HandleGetRequestQueue(const base::ListValue* args); On 2016/06/09 23:35:02, Dan Beam wrote: > typically these are all just named > HandleExactNameOfChromeSendOrCrSendWithPromise and also document arguments (or > that there are no arguments) Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:65: On 2016/06/09 23:35:02, Dan Beam wrote: > doc comment Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:68: On 2016/06/09 23:35:01, Dan Beam wrote: > doc comment Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:73: offline_pages::OfflinePageModel* offline_page_model_; On 2016/06/09 23:35:02, Dan Beam wrote: > where is this initialized originally? initialized in RegisterMessages, because that's when we can guarantee that web_ui() is set https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:73: offline_pages::OfflinePageModel* offline_page_model_; On 2016/06/09 23:35:02, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:89: offline_pages::DeletePageResult value) { On 2016/06/09 23:35:01, Dan Beam wrote: > nit: I think this implementation would be more future-proof and typesafe: > > > std::string OfflineInternalsUIMessageHandler::DeletePageResultToString( > offline_pages::DeletePageResult result) { > switch (result) { > case SomeResult: > return "Some string"; > ... > } > NOTREACHED(); > return "Unknown"; > } Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:95: else On 2016/06/09 23:35:02, Dan Beam wrote: > no else after return Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:101: const std::string kRequestStatusToString[] = { On 2016/06/09 23:36:30, dpapad wrote: > This constant array is identical with line 91. Should it be moved outside the > function and shared between the two functions? when i copied/pasted due to a previous comment, copied wrong thing :( Fixed now! https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:106: else On 2016/06/09 23:35:02, Dan Beam wrote: > no else after return Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:112: const base::Value* callback_id; On 2016/06/09 23:35:02, Dan Beam wrote: > what's the ownership of |callback_id|? is it safe to pass into base::Bind() > like this? I'm a bit rusty on C++ here. this won't compile when I passed in *callback_id and made the parameter type const base::Value&... I believe *callback_id is allocated on the heap. When I looked at all the value creation stuff, it seems like they were created by calling new <valueType> https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:113: args->Get(0, &callback_id); On 2016/06/09 23:36:30, dpapad wrote: > CHECK(args->Get(0, &callback_id)); Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:134: offline_ids.push_back(atoll(value.c_str())); On 2016/06/09 23:35:01, Dan Beam wrote: > is this different/better than base::StringToInt64()? > > https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.h... ooh, didn't know about StringToInt64. Using that now. I don't think it's different or better. It's just part of the default library https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:146: On 2016/06/09 23:36:30, dpapad wrote: > Nit: No need for \n here. Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:204: if (request_coordinator_ != nullptr) On 2016/06/09 23:35:01, Dan Beam wrote: > nit: curlies around 2+ line ifs Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:204: if (request_coordinator_ != nullptr) On 2016/06/09 23:35:02, Dan Beam wrote: > nit: if (request_coordinator_) seems equivalent but shorter Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:204: if (request_coordinator_ != nullptr) On 2016/06/09 23:36:30, dpapad wrote: > The Promise that JS holds is never fulfilled (resolved or rejected) if this > condition is false. Can we handle this case such that no Promise is left > indefinitely un-fulfiled. Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:205: request_coordinator_->queue()->GetRequests( On 2016/06/09 23:36:30, dpapad wrote: > Nit: Perhaps more readable to use curly braces for this if block, since it spans > multiple lines. Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:221: weak_ptr_factory_.GetWeakPtr(), callback_id)); On 2016/06/09 23:36:30, dpapad wrote: > Same here, what happens to the Promise? Done. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:234: "getRequestQueueInfo", On 2016/06/09 23:35:02, Dan Beam wrote: > nit: could this be just "getRequestQueue"? Renamed the method, so keeping as is https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:238: "getStoredPagesInfo", On 2016/06/09 23:35:02, Dan Beam wrote: > could this just be "getStoredPages"? Acknowledged.
Will take a closer final look tomorrow morning. https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:112: const base::Value* callback_id; On 2016/06/10 at 01:04:52, chili wrote: > On 2016/06/09 23:35:02, Dan Beam wrote: > > what's the ownership of |callback_id|? is it safe to pass into base::Bind() > > like this? > > I'm a bit rusty on C++ here. this won't compile when I passed in *callback_id and made the parameter type const base::Value&... > > I believe *callback_id is allocated on the heap. When I looked at all the value creation stuff, it seems like they were created by calling new <valueType> You can follow this example https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/about_h... to get the callback_id as an std::string, instead of a pointer and pass it around by value instead, to avoid leaking memory.
On 2016/06/10 01:25:09, dpapad wrote: > Will take a closer final look tomorrow morning. > > https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/offline_internals_ui.cc (right): > > https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/ui/webui... > chrome/browser/ui/webui/offline_internals_ui.cc:112: const base::Value* > callback_id; > On 2016/06/10 at 01:04:52, chili wrote: > > On 2016/06/09 23:35:02, Dan Beam wrote: > > > what's the ownership of |callback_id|? is it safe to pass into base::Bind() > > > like this? > > > > I'm a bit rusty on C++ here. this won't compile when I passed in *callback_id > and made the parameter type const base::Value&... > > > > I believe *callback_id is allocated on the heap. When I looked at all the > value creation stuff, it seems like they were created by calling new <valueType> > > You can follow this example > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/about_h... > to get the callback_id as an std::string, instead of a pointer and pass it > around by value instead, to avoid leaking memory. Dan suggested in a previous CL that I should use Get(Value**)... I'll wait for his approval as well
https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:106: return "Moooooooo"; Probably should also NOTREACHED() in this branch here :) https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:133: CHECK(args->Get(0, &callback_id)); right, |callback_id| is owned by |args| (per the docs for base::ListValue at https://cs.chromium.org/chromium/src/base/values.h?rcl=0&l=421) , which may not outlive the callback from the OfflinePageModel. So you must not use callback_id in base::Bind. https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:231: ResolveJavascriptCallback(*callback_id, results); Possibly RejectJavascriptCallback?
> Dan suggested in a previous CL that I should use Get(Value**)... I'll wait for his approval as well FYI, Dan is OOO today.
On 2016/06/10 18:23:24, dpapad wrote: > > Dan suggested in a previous CL that I should use Get(Value**)... I'll wait for > his approval as well > > FYI, Dan is OOO today. Seems like the Value** isn't safe anyway. I will go ahead with the change.
I think I replied to all the comments... Thanks! https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {Array<OfflinePageItem>} */ On 2016/06/10 00:47:04, dpapad wrote: > FYI, it seems that non-nullable is the default for typedefs, see example below. > > https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClo... > > I agree that the nullability concept of the JS compiler is confusing, since some > types are nullable by default, and others are not (string, number, boolean). I > would prefer having consistent nullability semantics values for all types. removed the ! https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:106: return "Moooooooo"; On 2016/06/10 18:14:33, dewittj wrote: > Probably should also NOTREACHED() in this branch here :) Done. https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:133: CHECK(args->Get(0, &callback_id)); On 2016/06/10 18:14:33, dewittj wrote: > right, |callback_id| is owned by |args| (per the docs for base::ListValue at > https://cs.chromium.org/chromium/src/base/values.h?rcl=0&l=421) > , which may not outlive the callback from the OfflinePageModel. So you must not > use callback_id in base::Bind. Done. https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:231: ResolveJavascriptCallback(*callback_id, results); On 2016/06/10 18:14:33, dewittj wrote: > Possibly RejectJavascriptCallback? I think it could go either way. Perhaps there shouldn't be a request coordinator for the profile being viewed (like incognito or something), in which case we don't want an error, just an empty page?
LGTM with a comment on my part. I suggest still waiting on dbeam's approval, since there were quite a few comments. https://codereview.chromium.org/2038963002/diff/120001/content/browser/webui/... File content/browser/webui/web_ui_message_handler.cc (right): https://codereview.chromium.org/2038963002/diff/120001/content/browser/webui/... content/browser/webui/web_ui_message_handler.cc:84: void WebUIMessageHandler::ResolveJavascriptCallback( I would prefer to not overload ResolveJavascriptCallback(). If you think this version is more useful, then we can maybe allow only this version, but still this belongs better in a separate CL (you need extra OWNERS approval just because of touching this file). For now just call base::StringValue() from your handler directly, like https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/font_ha....
On 2016/06/10 21:08:55, dpapad wrote: > LGTM with a comment on my part. I suggest still waiting on dbeam's approval, > since there were quite a few comments. > > https://codereview.chromium.org/2038963002/diff/120001/content/browser/webui/... > File content/browser/webui/web_ui_message_handler.cc (right): > > https://codereview.chromium.org/2038963002/diff/120001/content/browser/webui/... > content/browser/webui/web_ui_message_handler.cc:84: void > WebUIMessageHandler::ResolveJavascriptCallback( > I would prefer to not overload ResolveJavascriptCallback(). If you think this > version is more useful, then we can maybe allow only this version, but still > this belongs better in a separate CL (you need extra OWNERS approval just > because of touching this file). > > For now just call base::StringValue() from your handler directly, like > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/font_ha.... Reverted. Also, can you advise on this presubmit warning? When updating or adding third party code the appropriate 'README.chromium' file should also be updated with the correct version and package information. Thanks
> When updating or adding third party code the appropriate 'README.chromium' file should also be updated with the correct version and package information. The warning comes from https://cs.chromium.org/chromium/src/third_party/PRESUBMIT.py?l=55-59. You can ignore it, since you are simply updating a compiled_resources2.gyp file, and not the actual third party code.
https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:184: } On 2016/06/10 01:04:51, chili wrote: > On 2016/06/09 23:35:01, Dan Beam wrote: > > is there any particular reason you want to do this in JS instead of in C++? > > > > have you seen: components/password_manager/core/browser/export/csv_writer.h > > > > maybe this could be shared? > > > > also, if you do end up doing this in JS: maybe add a little OO to this code, > > i.e. make a CSVWriter class that can hide some of these details (i.e. > escaping, > > line joining, etc.)? > > Not knowing too much about webui and how to get downloads to call into C++ with > a filepath, I thought to get something working first. Will probably ping you > about that part. > > If alright with you, I'd also like to push creating CSVWriter to the next CL. > I'm planning to use the proxy stuff I talked with dpapad@ in the next CL, which > is going to involve restructuring this class a bit anyway. this doesn't make much sense. why would you write a CSV generator just to remove it and use the pre-written one? https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:17: var OfflinePageItem; OfflinePage https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:35: var currentStoredPagesData = null; currentStoredPagesData -> offlinePages? https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:38: var currentRequestsData = null; savePageRequests? https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:150: for (let obj of currentStoredPagesData) { let page of currentStoredPages https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:151: var objFieldArr = [ objFieldArr -> csvFields or csvLine{Values} or something? https://codereview.chromium.org/2038963002/diff/120001/content/browser/webui/... File content/browser/webui/web_ui_message_handler.cc (right): https://codereview.chromium.org/2038963002/diff/120001/content/browser/webui/... content/browser/webui/web_ui_message_handler.cc:84: void WebUIMessageHandler::ResolveJavascriptCallback( On 2016/06/10 21:08:55, dpapad wrote: > I would prefer to not overload ResolveJavascriptCallback(). If you think this > version is more useful, then we can maybe allow only this version, but still > this belongs better in a separate CL (you need extra OWNERS approval just > because of touching this file). > > For now just call base::StringValue() from your handler directly, like > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/font_ha.... the C++ style guide discourages overloading: https://google.github.io/styleguide/cppguide.html#Function_Overloading
https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:184: } On 2016/06/13 19:13:28, Dan Beam wrote: > On 2016/06/10 01:04:51, chili wrote: > > On 2016/06/09 23:35:01, Dan Beam wrote: > > > is there any particular reason you want to do this in JS instead of in C++? > > > > > > have you seen: components/password_manager/core/browser/export/csv_writer.h > > > > > > maybe this could be shared? > > > > > > also, if you do end up doing this in JS: maybe add a little OO to this code, > > > i.e. make a CSVWriter class that can hide some of these details (i.e. > > escaping, > > > line joining, etc.)? > > > > Not knowing too much about webui and how to get downloads to call into C++ > with > > a filepath, I thought to get something working first. Will probably ping you > > about that part. > > > > If alright with you, I'd also like to push creating CSVWriter to the next CL. > > I'm planning to use the proxy stuff I talked with dpapad@ in the next CL, > which > > is going to involve restructuring this class a bit anyway. > > this doesn't make much sense. why would you write a CSV generator just to > remove it and use the pre-written one? Right now I prefer to keep the CSV generation in JS. You suggested extracting parts of this function out into a CSVWriter class to hide some of the details if we end up keeping this in JS. I'm hoping that I can do this in my next iteration where I'm going to do some purely invisible enhancements (i.e. using browser proxy, etc). This is to get a useable version of the internals page to my team faster. I think there may be some differing opinions of whether the CSV generation should be done in JS vs C++, and I'd like some time to do some playing around with both the C++ csv_writer.h and writing my own JS-based CSVWriter class https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:17: var OfflinePageItem; On 2016/06/13 19:13:29, Dan Beam wrote: > OfflinePage Done. https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:35: var currentStoredPagesData = null; On 2016/06/13 19:13:28, Dan Beam wrote: > currentStoredPagesData -> offlinePages? Done. https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:38: var currentRequestsData = null; On 2016/06/13 19:13:29, Dan Beam wrote: > savePageRequests? Done. https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:150: for (let obj of currentStoredPagesData) { On 2016/06/13 19:13:29, Dan Beam wrote: > let page of currentStoredPages Done. https://codereview.chromium.org/2038963002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:151: var objFieldArr = [ On 2016/06/13 19:13:29, Dan Beam wrote: > objFieldArr -> csvFields or csvLine{Values} or something? Done.
https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:184: } On 2016/06/13 20:47:14, chili wrote: > On 2016/06/13 19:13:28, Dan Beam wrote: > > On 2016/06/10 01:04:51, chili wrote: > > > On 2016/06/09 23:35:01, Dan Beam wrote: > > > > is there any particular reason you want to do this in JS instead of in > C++? > > > > > > > > have you seen: > components/password_manager/core/browser/export/csv_writer.h > > > > > > > > maybe this could be shared? > > > > > > > > also, if you do end up doing this in JS: maybe add a little OO to this > code, > > > > i.e. make a CSVWriter class that can hide some of these details (i.e. > > > escaping, > > > > line joining, etc.)? > > > > > > Not knowing too much about webui and how to get downloads to call into C++ > > with > > > a filepath, I thought to get something working first. Will probably ping you > > > about that part. > > > > > > If alright with you, I'd also like to push creating CSVWriter to the next > CL. > > > I'm planning to use the proxy stuff I talked with dpapad@ in the next CL, > > which > > > is going to involve restructuring this class a bit anyway. > > > > this doesn't make much sense. why would you write a CSV generator just to > > remove it and use the pre-written one? > > Right now I prefer to keep the CSV generation in JS. Is there a reason other than your preference? Why is Chrome better off with another CSV writer in another language, written in a way that cannot be shared? > You suggested extracting > parts of this function out into a CSVWriter class to hide some of the details if > we end up keeping this in JS. No, I recommended using the existing one[1] that's already implemented and has tests or explaining clearly why your JavaScript implementation is better. I also think your code could be structured in a more flexible way and better tested. Chromium is a huge codebase with many things to offer; common tasks (i.e. export to CSV) have often been done for you. [1] https://cs.chromium.org/chromium/src/components/password_manager/core/browser... > I'm hoping that I can do this in my next > iteration where I'm going to do some purely invisible enhancements (i.e. using > browser proxy, etc). This is to get a useable version of the internals page to > my team faster. > > I think there may be some differing opinions of whether the CSV generation > should be done in JS vs C++, and I'd like some time to do some playing around > with both the C++ csv_writer.h and writing my own JS-based CSVWriter class
https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/80001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:184: } On 2016/06/13 21:13:23, Dan Beam wrote: > On 2016/06/13 20:47:14, chili wrote: > > On 2016/06/13 19:13:28, Dan Beam wrote: > > > On 2016/06/10 01:04:51, chili wrote: > > > > On 2016/06/09 23:35:01, Dan Beam wrote: > > > > > is there any particular reason you want to do this in JS instead of in > > C++? > > > > > > > > > > have you seen: > > components/password_manager/core/browser/export/csv_writer.h > > > > > > > > > > maybe this could be shared? > > > > > > > > > > also, if you do end up doing this in JS: maybe add a little OO to this > > code, > > > > > i.e. make a CSVWriter class that can hide some of these details (i.e. > > > > escaping, > > > > > line joining, etc.)? > > > > > > > > Not knowing too much about webui and how to get downloads to call into C++ > > > with > > > > a filepath, I thought to get something working first. Will probably ping > you > > > > about that part. > > > > > > > > If alright with you, I'd also like to push creating CSVWriter to the next > > CL. > > > > I'm planning to use the proxy stuff I talked with dpapad@ in the next CL, > > > which > > > > is going to involve restructuring this class a bit anyway. > > > > > > this doesn't make much sense. why would you write a CSV generator just to > > > remove it and use the pre-written one? > > > > Right now I prefer to keep the CSV generation in JS. > > Is there a reason other than your preference? Why is Chrome better off with > another CSV writer in another language, written in a way that cannot be shared? > > > You suggested extracting > > parts of this function out into a CSVWriter class to hide some of the details > if > > we end up keeping this in JS. > > No, I recommended using the existing one[1] that's already implemented and has > tests or explaining clearly why your JavaScript implementation is better. I > also think your code could be structured in a more flexible way and better > tested. > > Chromium is a huge codebase with many things to offer; common tasks (i.e. export > to CSV) have often been done for you. > > [1] > https://cs.chromium.org/chromium/src/components/password_manager/core/browser... > > > I'm hoping that I can do this in my next > > iteration where I'm going to do some purely invisible enhancements (i.e. using > > browser proxy, etc). This is to get a useable version of the internals page > to > > my team faster. > > > > I think there may be some differing opinions of whether the CSV generation > > should be done in JS vs C++, and I'd like some time to do some playing around > > with both the C++ csv_writer.h and writing my own JS-based CSVWriter class I was looking at the uses for csv_writer when you first suggested, which seems to involve a file finder and write to that path. I haven't quite figured out how to bring up this file finder from clicking on a javascript button and how all of that interaction would work yet. In addition, I'd probably have to move it out of components/password_manager so neither team will wonder about why offline internals is depending on password_manager. To me, this is an indication that a good amount of things may need to change, and even longer code review. On the other hand, I'd like to get this CL landed so my teammates will be able to start using the page even as I figure out how to do this whole other stuff. I understand your concerns about writing something that will be removed/changed in the next iteration. However, the difference in time between these may be a couple of weeks, which is a couple of weeks my teammates won't be able to use this page. My suggestion isn't saying that one way is better than another. Of course, out of laziness and being more comfortable debugging/working with JS than with C++, I would prefer not having to change at all... I'm just hoping to investigate these big changes to a different CL where time is less of a concern.
could we just use JSON instead? is that possible? the code would be JSON.stringify(), that's it :) https://codereview.chromium.org/2038963002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:35: var offlinePages = null; why is this null and not []? https://codereview.chromium.org/2038963002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:38: var savePageRequests = null; why not []? https://codereview.chromium.org/2038963002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:138: return '"' + strObj.replace(/"/g, '""') + '"'; what if there's a comma in the string? I am mentioning this because I saw it in csv_writer.cc's implementation and tests
As mentioned in offline discussion, changed to JSON download for now and switch to CSV later https://codereview.chromium.org/2038963002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:35: var offlinePages = null; On 2016/06/14 00:07:50, Dan Beam wrote: > why is this null and not []? Done. https://codereview.chromium.org/2038963002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:38: var savePageRequests = null; On 2016/06/14 00:07:50, Dan Beam wrote: > why not []? Done. https://codereview.chromium.org/2038963002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:138: return '"' + strObj.replace(/"/g, '""') + '"'; On 2016/06/14 00:07:50, Dan Beam wrote: > what if there's a comma in the string? > > I am mentioning this because I saw it in csv_writer.cc's implementation and > tests That is why we wrap the entire string with quotes and escape any quotes the string may already have... the csv_writer only puts quotes if there are commas in the string, which is more efficient, but this is no less correct
looking better! https://codereview.chromium.org/2038963002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:138: return '"' + strObj.replace(/"/g, '""') + '"'; On 2016/06/14 04:45:21, chili wrote: > On 2016/06/14 00:07:50, Dan Beam wrote: > > what if there's a comma in the string? > > > > I am mentioning this because I saw it in csv_writer.cc's implementation and > > tests > > That is why we wrap the entire string with quotes and escape any quotes the > string may already have... > > the csv_writer only puts quotes if there are commas in the string, which is more > efficient, but this is no less correct ah, ok, cool https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {?Array<OfflinePage>} */ this should be !Array<!OfflinePage> https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:37: /** @type {?Array<SavePageRequest>} */ same here, !Array<!SavePageRequest> https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:70: cell.textContent = Math.round(pages[i].size / 1024); should this be using ui::FormatBytes()? https://cs.chromium.org/chromium/src/ui/base/text/bytes_formatting.h?q=base+b... https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:150: var uriContent = 'data:text/json,' + encodeURIComponent(json); application/json? https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:150: var uriContent = 'data:text/json,' + encodeURIComponent(json); using encodeURIComponent seems a little dubious... https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:77: offline_pages::OfflinePageModel* offline_page_model_; this should be initialized to nullptr https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:80: offline_pages::RequestCoordinator* request_coordinator_; this should be initialized to nullptr https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:108: return "Moooooooo"; // Cows, because why not. nit: just put a break; here and fall through to the NOTREACHED() below https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:156: int64_t intValue; int_value https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:181: base::DictionaryValue* js_page_object = new base::DictionaryValue(); js_page_object -> offline_page https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:200: base::ListValue js_requests; save_page_request or page_request https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:203: base::DictionaryValue* js_request_object = new base::DictionaryValue(); page_request or save_page_request https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:227: request_coordinator_->queue()->GetRequests(base::Bind( nit: can request_coordinator_ exist but without request_coordinator_->queue()?
https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {?Array<OfflinePage>} */ On 2016/06/14 15:43:37, Dan Beam wrote: > this should be !Array<!OfflinePage> Comment from dpapad@ in the previous CL mentioned that closure defaults types inside arrays/collections to not null, which means the ! is not needed. https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:37: /** @type {?Array<SavePageRequest>} */ On 2016/06/14 15:43:37, Dan Beam wrote: > same here, !Array<!SavePageRequest> same as above https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:70: cell.textContent = Math.round(pages[i].size / 1024); On 2016/06/14 15:43:37, Dan Beam wrote: > should this be using ui::FormatBytes()? > > https://cs.chromium.org/chromium/src/ui/base/text/bytes_formatting.h?q=base+b... I didn't because format byte would return a string. Though I want to display the size in kb, I still want the download to be the full number. If this is a big problem, I'll just revert the byte -> kb https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:150: var uriContent = 'data:text/json,' + encodeURIComponent(json); On 2016/06/14 15:43:37, Dan Beam wrote: > application/json? Done. https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:150: var uriContent = 'data:text/json,' + encodeURIComponent(json); On 2016/06/14 15:43:37, Dan Beam wrote: > using encodeURIComponent seems a little dubious... Done. https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:77: offline_pages::OfflinePageModel* offline_page_model_; On 2016/06/14 15:43:37, Dan Beam wrote: > this should be initialized to nullptr Done. https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:80: offline_pages::RequestCoordinator* request_coordinator_; On 2016/06/14 15:43:37, Dan Beam wrote: > this should be initialized to nullptr Done. https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:108: return "Moooooooo"; // Cows, because why not. On 2016/06/14 15:43:37, Dan Beam wrote: > nit: just put a break; here and fall through to the NOTREACHED() below Done. https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:156: int64_t intValue; On 2016/06/14 15:43:37, Dan Beam wrote: > int_value Done. https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:181: base::DictionaryValue* js_page_object = new base::DictionaryValue(); On 2016/06/14 15:43:37, Dan Beam wrote: > js_page_object -> offline_page Done. https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:200: base::ListValue js_requests; On 2016/06/14 15:43:37, Dan Beam wrote: > save_page_request or page_request Done. https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:203: base::DictionaryValue* js_request_object = new base::DictionaryValue(); On 2016/06/14 15:43:37, Dan Beam wrote: > page_request or save_page_request Done. https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:227: request_coordinator_->queue()->GetRequests(base::Bind( On 2016/06/14 15:43:37, Dan Beam wrote: > nit: can request_coordinator_ exist but without request_coordinator_->queue()? nope. a queue is required to construct a request coordinator
https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:150: var uriContent = 'data:text/json,' + encodeURIComponent(json); On 2016/06/14 17:25:43, chili wrote: > On 2016/06/14 15:43:37, Dan Beam wrote: > > using encodeURIComponent seems a little dubious... > > Done. From RFC2397 for data URLs: Without ";base64", the data (as a sequence of octets) is represented using ASCII encoding for octets inside the range of safe URL characters and using the standard %xx hex encoding of URLs for octets outside that range. So you still need to encode the json. I think that encodeURIComponent is the right function here. Dan, what is dubious about that?
https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:34: /** @type {?Array<OfflinePage>} */ On 2016/06/14 17:25:43, chili wrote: > On 2016/06/14 15:43:37, Dan Beam wrote: > > this should be !Array<!OfflinePage> > > Comment from dpapad@ in the previous CL mentioned that closure defaults types > inside arrays/collections to not null, which means the ! is not needed. that's fine, but ? means "or null", which these variable no longer are
lgtm https://codereview.chromium.org/2038963002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/220001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:148: savePageRequests: savePageRequests}); fyi, both object and array literals in JS are often 2\s indented with the } or ] on the next line, i.e. var json = JSON.stringify({ offlinePages: offlinePages, savePageRequests: savePageRequests });
https://codereview.chromium.org/2038963002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/220001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:148: savePageRequests: savePageRequests}); On 2016/06/14 17:56:20, Dan Beam wrote: > fyi, both object and array literals in JS are often 2\s indented with the } or ] > on the next line, i.e. > > var json = JSON.stringify({ > offlinePages: offlinePages, > savePageRequests: savePageRequests > }); https://engdoc.corp.google.com/eng/doc/javascriptguide.xml?cl=head#Array_and_...
https://codereview.chromium.org/2038963002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/220001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:146: var json = JSON.stringify({ FYI, if you want the output JSON to be indented such that it is more human readable, you can do the following, JSON.stringify({...}, null, 2);
https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/180001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:150: var uriContent = 'data:text/json,' + encodeURIComponent(json); On 2016/06/14 17:32:54, dewittj wrote: > On 2016/06/14 17:25:43, chili wrote: > > On 2016/06/14 15:43:37, Dan Beam wrote: > > > using encodeURIComponent seems a little dubious... > > > > Done. > > From RFC2397 for data URLs: > Without ";base64", the data (as a sequence of > octets) is represented using ASCII encoding for octets inside the > range of safe URL characters and using the standard %xx hex encoding > of URLs for octets outside that range. > > So you still need to encode the json. I think that encodeURIComponent is the > right function here. > > Dan, what is dubious about that? I'm putting this back, since we're using data uri, and I'd rather the URL we generate be valid than fail for some odd reason. https://codereview.chromium.org/2038963002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2038963002/diff/220001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:146: var json = JSON.stringify({ On 2016/06/14 17:59:47, dpapad wrote: > FYI, if you want the output JSON to be indented such that it is more human > readable, you can do the following, > > JSON.stringify({...}, null, 2); Done. https://codereview.chromium.org/2038963002/diff/220001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:148: savePageRequests: savePageRequests}); On 2016/06/14 17:56:20, Dan Beam wrote: > fyi, both object and array literals in JS are often 2\s indented with the } or ] > on the next line, i.e. > > var json = JSON.stringify({ > offlinePages: offlinePages, > savePageRequests: savePageRequests > }); Done. https://codereview.chromium.org/2038963002/diff/220001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:148: savePageRequests: savePageRequests}); On 2016/06/14 17:56:40, Dan Beam wrote: > On 2016/06/14 17:56:20, Dan Beam wrote: > > fyi, both object and array literals in JS are often 2\s indented with the } or > ] > > on the next line, i.e. > > > > var json = JSON.stringify({ > > offlinePages: offlinePages, > > savePageRequests: savePageRequests > > }); > > https://engdoc.corp.google.com/eng/doc/javascriptguide.xml?cl=head#Array_and_... Acknowledged.
lgtm
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2038963002/#ps240001 (title: "small fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038963002/240001
fyi: <a href="" download></a> might be the best choice for "how does one force download a file from the page?" nothing to do in this CL, but changing your markup to: <a href="/dump.csv" download>Dump!</a> and making a URL handler in the C++ that responds to the "/dump.csv" path via writing a CSV is probably fairly easy/natural/straight-forward.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios-device-gn on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by chili@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038963002/240001
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Link the internals page with the offline model and request coordinators - Adds functionality to 'dump' all content into a csv file for download. - Adds adds the resource files to closure BUG=609570 DIFF_BASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Offline Pages] Link the internals page with the offline model and request coordinators - Adds functionality to 'dump' all content into a csv file for download. - Adds adds the resource files to closure BUG=609570 DIFF_BASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Link the internals page with the offline model and request coordinators - Adds functionality to 'dump' all content into a csv file for download. - Adds adds the resource files to closure BUG=609570 DIFF_BASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Offline Pages] Link the internals page with the offline model and request coordinators - Adds functionality to 'dump' all content into a csv file for download. - Adds adds the resource files to closure BUG=609570 DIFF_BASE=2003883002 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/27d6f0a481a7f73b7dd60f894000c8d22f735332 Cr-Commit-Position: refs/heads/master@{#399818} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/27d6f0a481a7f73b7dd60f894000c8d22f735332 Cr-Commit-Position: refs/heads/master@{#399818} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
