Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2003883002:
[Offline pages] Create offline internals page for Android (Closed)
|
Created:
4 years, 7 months ago by chili Modified:
4 years, 6 months ago CC:
dpapad, asvitkine+watch_chromium.org, chili+watch_chromium.org, chromium-reviews, dewittj+watch_chromium.org, dimich+watch_chromium.org, fgorski+watch_chromium.org, petewil+watch_chromium.org, romax+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Create offline internals page for android
BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/b9e16a70339082317d07db9f4165e10257723a28
Cr-Commit-Position: refs/heads/master@{#398394}
Patch Set 1 #Patch Set 2 : #
Total comments: 18
Patch Set 3 : Resolve cl comments #
Total comments: 22
Patch Set 4 : Resolve comments by jianli #Patch Set 5 : #Patch Set 6 : #
Total comments: 10
Patch Set 7 : minor build fix and addressing comments #Patch Set 8 : #
Total comments: 3
Patch Set 9 : #
Total comments: 36
Patch Set 10 : resolve comments #Patch Set 11 : Fix minor spelling mishaps #
Total comments: 33
Patch Set 12 : Addressing comments left by dbeam@: better use of closure annotations and use cr.sendWithPromise #Patch Set 13 : #
Total comments: 14
Patch Set 14 : addressing more comments #
Total comments: 8
Patch Set 15 : merge #Patch Set 16 : #Messages
Total messages: 36 (10 generated)
chili@chromium.org changed reviewers: + dewittj@chromium.org, fgorski@chromium.org
This is simply the hookups necessary to show an internal page, with some initial data rendering. Real page content will come in a separate CL
https://codereview.chromium.org/2003883002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.h (right): https://codereview.chromium.org/2003883002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.h:12: class OfflineInternalsUI : public content::WebUIController { are internals UIs kept in top level namespace? https://codereview.chromium.org/2003883002/diff/20001/components/offline_page... File components/offline_pages/resources/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/20001/components/offline_page... components/offline_pages/resources/offline_internals.js:95: // does nothing yet. add TODO and explain what will be done here. https://codereview.chromium.org/2003883002/diff/20001/components/offline_page... components/offline_pages/resources/offline_internals.js:102: refreshTable('stored-pages'); this sounds more like clearTable. https://codereview.chromium.org/2003883002/diff/20001/components/offline_page... components/offline_pages/resources/offline_internals.js:112: function clearSelectedPages() { would delete selected pages be more suitable here? Also will this expire a page or remove a page completely? (This impacts omnibox behavior on the tab opened to that page, hence my question. Also. Do you plan to show items that are expired in the list or not? https://codereview.chromium.org/2003883002/diff/20001/components/resources/OW... File components/resources/OWNERS (right): https://codereview.chromium.org/2003883002/diff/20001/components/resources/OW... components/resources/OWNERS:25: per-file offline_pages_resources.grdp=peter@chromium.org petewil, if you mean Peter on our team https://codereview.chromium.org/2003883002/diff/20001/components/resources/OW... components/resources/OWNERS:26: per-file offline_pages_resources.grdp=zea@chromium.org You don't need zea@
https://codereview.chromium.org/2003883002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2003883002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:53: web_ui()->CallJavascriptFunction("offlineInternals.pagesCleared"); I don't know enough about WebUIMessageHandler to know: will this actually call pagesCleared in the same callstack as clearPages? That could become confusing to develop with. https://codereview.chromium.org/2003883002/diff/20001/components/offline_page... File components/offline_pages/resources/offline_internals.css (right): https://codereview.chromium.org/2003883002/diff/20001/components/offline_page... components/offline_pages/resources/offline_internals.css:6: h1 { is this style copied from somewhere? I would prefer if you use the suggestions in https://www.chromium.org/developers/updating-webui-for-material-design https://codereview.chromium.org/2003883002/diff/20001/components/resources/OW... File components/resources/OWNERS (right): https://codereview.chromium.org/2003883002/diff/20001/components/resources/OW... components/resources/OWNERS:21: per-file offline_pages_resources.grdp=chili@chromium.org I don't think that non-committers can be OWNERS. So I'd just include the rest of the offline pages team (well, the Chromium committers) here in order to minimize commit friction. dewittj dimich fgorski jianli petewil Justin
https://codereview.chromium.org/2003883002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2003883002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:53: web_ui()->CallJavascriptFunction("offlineInternals.pagesCleared"); On 2016/05/24 18:13:08, dewittj wrote: > I don't know enough about WebUIMessageHandler to know: will this actually call > pagesCleared in the same callstack as clearPages? That could become confusing > to develop with. From what I gather, when the javascript says "call function x", it actually makes an async request which eventually lands to this handler, which kind of acts as a 'server'. The web_ui()->CallJavascriptFunction is basically like calling the callback function in an async request, so it's not the same callstack https://codereview.chromium.org/2003883002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.h (right): https://codereview.chromium.org/2003883002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.h:12: class OfflineInternalsUI : public content::WebUIController { On 2016/05/24 17:50:18, fgorski wrote: > are internals UIs kept in top level namespace? It appears to be so. None of the other internal UIs declare a namespace. https://codereview.chromium.org/2003883002/diff/20001/components/offline_page... File components/offline_pages/resources/offline_internals.css (right): https://codereview.chromium.org/2003883002/diff/20001/components/offline_page... components/offline_pages/resources/offline_internals.css:6: h1 { On 2016/05/24 18:13:08, dewittj wrote: > is this style copied from somewhere? I would prefer if you use the suggestions > in https://www.chromium.org/developers/updating-webui-for-material-design Polymer is just a way to more easily split html templates into multiple smaller templates, and embed useful templating in the html. I spoke with dbeam@ (one of the owners of webui), who said that polymer in internal pages does not yet work with android (apparently adds a lot to the size). Since this will be primarily viewed on android, I will not be adding polymer. That said, this CSS doesn't really conform to material specs, but neither is there a 'default material css' provided. For a first pass, I think it's more important to have all the information in some readable format. I intend to do a CSS pass later for a purely visual touchup https://codereview.chromium.org/2003883002/diff/20001/components/offline_page... File components/offline_pages/resources/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/20001/components/offline_page... components/offline_pages/resources/offline_internals.js:95: // does nothing yet. On 2016/05/24 17:50:18, fgorski wrote: > add TODO and explain what will be done here. Done. https://codereview.chromium.org/2003883002/diff/20001/components/offline_page... components/offline_pages/resources/offline_internals.js:102: refreshTable('stored-pages'); On 2016/05/24 17:50:18, fgorski wrote: > this sounds more like clearTable. Done. https://codereview.chromium.org/2003883002/diff/20001/components/offline_page... components/offline_pages/resources/offline_internals.js:112: function clearSelectedPages() { On 2016/05/24 17:50:18, fgorski wrote: > would delete selected pages be more suitable here? > Also will this expire a page or remove a page completely? (This impacts omnibox > behavior on the tab opened to that page, hence my question. > > Also. Do you plan to show items that are expired in the list or not? Renamed clear -> delete. This will eventually call the ClearAll() or DeletePageByOfflineId() methods on the offlinePageModel, so will do whatever it does (from method documentation, seems to remove a page completely) Items shown are from the GetAllPages() method on the OfflinePageModel, which it seems like will show expired but not deleted pages if there are any. https://codereview.chromium.org/2003883002/diff/20001/components/resources/OW... File components/resources/OWNERS (right): https://codereview.chromium.org/2003883002/diff/20001/components/resources/OW... components/resources/OWNERS:21: per-file offline_pages_resources.grdp=chili@chromium.org On 2016/05/24 18:13:08, dewittj wrote: > I don't think that non-committers can be OWNERS. So I'd just include the rest > of the offline pages team (well, the Chromium committers) here in order to > minimize commit friction. > > dewittj > dimich > fgorski > jianli > petewil > > Justin Done. https://codereview.chromium.org/2003883002/diff/20001/components/resources/OW... components/resources/OWNERS:25: per-file offline_pages_resources.grdp=peter@chromium.org On 2016/05/24 17:50:18, fgorski wrote: > petewil, if you mean Peter on our team Updated to just you, justin, dmitry, and me. I figure the others can add themselves to the owners as needed https://codereview.chromium.org/2003883002/diff/20001/components/resources/OW... components/resources/OWNERS:26: per-file offline_pages_resources.grdp=zea@chromium.org On 2016/05/24 17:50:18, fgorski wrote: > You don't need zea@ Acknowledged.
jianli@chromium.org changed reviewers: + jianli@chromium.org
https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:80: "//components/offline_pages", Do we really want this? https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:36: void DeleteSelectPages(const base::ListValue* args); nit: DeleteSelectedPages https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:64: void Will this get compiled? https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:74: base::DictionaryValue* samplePage = new base::DictionaryValue(); Try not to land the CL with dummy code. You could simply keep them for your private testing. https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:115: // Required resources nit: end with period https://codereview.chromium.org/2003883002/diff/40001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/2003883002/diff/40001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:356: 'browser/ui/webui/offline_internals_ui.cc', Move these 2 files to chrome_browser_ui_android_java_ui_sources. https://codereview.chromium.org/2003883002/diff/40001/chrome/common/url_const... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2003883002/diff/40001/chrome/common/url_const... chrome/common/url_constants.cc:214: const char kChromeUIOfflineInternalsHost[] = "offline-internals"; Consider wrapping this under OS_ANDROID https://codereview.chromium.org/2003883002/diff/40001/components/offline_page... File components/offline_pages/resources/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/40001/components/offline_page... components/offline_pages/resources/offline_internals.js:20: * Refresh the tables Comment is not consistent with the operation below. https://codereview.chromium.org/2003883002/diff/40001/components/offline_page... components/offline_pages/resources/offline_internals.js:34: if (element == null) { nit: be consistent with line 24 https://codereview.chromium.org/2003883002/diff/40001/components/offline_page... components/offline_pages/resources/offline_internals.js:62: if (element == null) { ditto https://codereview.chromium.org/2003883002/diff/40001/components/resources/co... File components/resources/components_resources.grd (right): https://codereview.chromium.org/2003883002/diff/40001/components/resources/co... components/resources/components_resources.grd:18: <part file="offline_pages_resources.grdp" /> I think you should add these under chrome/browser/browser_resources.grd within is_android scope.
Description was changed from ========== [Offline pages] Create offline internals page with dummy data BUG=609570 ========== to ========== [Offline pages] Create offline internals page with dummy data BUG=609570 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:80: "//components/offline_pages", On 2016/05/25 00:17:17, jianli wrote: > Do we really want this? Currently this dependency isn't needed, but when actual data is fetched, we will need classes (OfflinePageModel and OfflinePageItem at minimum) from this directory. https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:36: void DeleteSelectPages(const base::ListValue* args); On 2016/05/25 00:17:17, jianli wrote: > nit: DeleteSelectedPages Done. https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:64: void On 2016/05/25 00:17:17, jianli wrote: > Will this get compiled? I just did a git cl format... seemed to have messed this part up :( https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:74: base::DictionaryValue* samplePage = new base::DictionaryValue(); On 2016/05/25 00:17:17, jianli wrote: > Try not to land the CL with dummy code. You could simply keep them for your > private testing. Acknowledged. https://codereview.chromium.org/2003883002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:115: // Required resources On 2016/05/25 00:17:17, jianli wrote: > nit: end with period Done. https://codereview.chromium.org/2003883002/diff/40001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/2003883002/diff/40001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:356: 'browser/ui/webui/offline_internals_ui.cc', On 2016/05/25 00:17:17, jianli wrote: > Move these 2 files to chrome_browser_ui_android_java_ui_sources. Done. https://codereview.chromium.org/2003883002/diff/40001/chrome/common/url_const... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2003883002/diff/40001/chrome/common/url_const... chrome/common/url_constants.cc:214: const char kChromeUIOfflineInternalsHost[] = "offline-internals"; On 2016/05/25 00:17:17, jianli wrote: > Consider wrapping this under OS_ANDROID Done. https://codereview.chromium.org/2003883002/diff/40001/components/offline_page... File components/offline_pages/resources/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/40001/components/offline_page... components/offline_pages/resources/offline_internals.js:20: * Refresh the tables On 2016/05/25 00:17:17, jianli wrote: > Comment is not consistent with the operation below. Done. https://codereview.chromium.org/2003883002/diff/40001/components/offline_page... components/offline_pages/resources/offline_internals.js:34: if (element == null) { On 2016/05/25 00:17:17, jianli wrote: > nit: be consistent with line 24 Done. https://codereview.chromium.org/2003883002/diff/40001/components/offline_page... components/offline_pages/resources/offline_internals.js:62: if (element == null) { On 2016/05/25 00:17:17, jianli wrote: > ditto Done. https://codereview.chromium.org/2003883002/diff/40001/components/resources/co... File components/resources/components_resources.grd (right): https://codereview.chromium.org/2003883002/diff/40001/components/resources/co... components/resources/components_resources.grd:18: <part file="offline_pages_resources.grdp" /> On 2016/05/25 00:17:17, jianli wrote: > I think you should add these under chrome/browser/browser_resources.grd within > is_android scope. Done.
Description was changed from ========== [Offline pages] Create offline internals page with dummy data BUG=609570 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Offline pages] Create offline internals page for android BUG=609570 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
lgtm Please also update the issue description, esp. "with dummy data" part. https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:342: nit: new line not needed https://codereview.chromium.org/2003883002/diff/100001/chrome/common/url_cons... File chrome/common/url_constants.h (right): https://codereview.chromium.org/2003883002/diff/100001/chrome/common/url_cons... chrome/common/url_constants.h:87: extern const char kChromeUIUserManagerURL[]; Why did you add this?
https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:13: * @param {HTMLElement} element A HTML element Is this being compiled by closure compiler? I recall hearing that that was the case a while ago. If so, please annotate all functions with @param and @return where possible. https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:68: for (var key in queueAttributes) { I don't like using var..in loops for arrays, since the key will be a numeric index, right? Clearer to use for (var i = 0; i < foo...
chili@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:13: * @param {HTMLElement} element A HTML element On 2016/05/25 20:08:45, dewittj wrote: > Is this being compiled by closure compiler? I recall hearing that that was the > case a while ago. If so, please annotate all functions with @param and @return > where possible. Done. https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:68: for (var key in queueAttributes) { On 2016/05/25 20:08:44, dewittj wrote: > I don't like using var..in loops for arrays, since the key will be a numeric > index, right? Clearer to use for (var i = 0; i < foo... Data is actually of type List<Map<String, Object>>, so 'key' is not numeric, but the string key value to the map if we change to for (var j = 0; j < foo...), then we would have cell.textContent = data[i][queueAttributes[j]], which is more confusing I think. If you still prefer an explicit for loop, I can change it https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:342: On 2016/05/25 20:04:06, jianli wrote: > nit: new line not needed Done. https://codereview.chromium.org/2003883002/diff/100001/chrome/common/url_cons... File chrome/common/url_constants.h (right): https://codereview.chromium.org/2003883002/diff/100001/chrome/common/url_cons... chrome/common/url_constants.h:87: extern const char kChromeUIUserManagerURL[]; On 2016/05/25 20:04:06, jianli wrote: > Why did you add this? I didn't add this. It was out of place in the alphabetization so I moved it to the correct place
chili@chromium.org changed reviewers: + holte@chromium.org
https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:68: for (var key in queueAttributes) { What I'm trying to say is that you already should be doing data[i][queueAttributes[key]] to get the correct behavior. I ran this code in the console: var queueAttributes = ['onlineUrl', 'creation time', 'status']; for (var k in queueAttributes) { console.log(k); } 0 1 2 Not only is k numeric, but it also includes other things that come along on Array.prototype (so should also test for hasOwnProperty) Am I misunderstanding something?
histograms lgtm
https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/100001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:68: for (var key in queueAttributes) { On 2016/05/25 20:30:13, dewittj wrote: > What I'm trying to say is that you already should be doing > data[i][queueAttributes[key]] to get the correct behavior. I ran this code in > the console: > > var queueAttributes = ['onlineUrl', 'creation time', 'status']; for (var k in > queueAttributes) { console.log(k); } > 0 > 1 > 2 > > Not only is k numeric, but it also includes other things that come along on > Array.prototype (so should also test for hasOwnProperty) > > Am I misunderstanding something? Ah. I misunderstood your comment. Yes, I overlooked the fact that for-in loops in javascript iterate over the enumerable properties of Array (which end up being its index, and also in non-deterministic order >"< ugh javascript). Fixed now. Thanks for catching this!
lgtm Closure stuff can be punted to a later cl if you like. https://codereview.chromium.org/2003883002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/140001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. I think you have not added this to compiled resources: https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat... It's okay to skip this for now but it'd be nice to get this to use the compilation tools we have now. https://codereview.chromium.org/2003883002/diff/140001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:32: * @param {HTMLElement} element A HTML element @param {?HTMLElement} if nullable, does $() return undefined or null if there's no match? https://codereview.chromium.org/2003883002/diff/140001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:33: * @param {Object} data An array object representing stored offline pages If i understand data right, this should be: @param {!Array<!Object<String,String>>} data ...
https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.css (right): https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.css:3: * found in the LICENSE file. nit: * found in the LICENSE file. */ ^^ https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:6: <if expr="is_android"> nit: <if> should arguably be 0-indented like #if in C++, as in: <if expr="is_android"> <meta name="viewport" content="width=device-width"> </if> https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:34: <th> </th> why is this necessary? https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:8: var storedPagesAttributes = ['onlineUrl', 'namespace', 'size']; nit: FINAL_CONST_LIKE_THIS (i.e. STORED_PAGE_ATTRIBUTES) https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:25: return; shouldn't this blow up because of programmer error when $(tableId) returns null? https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:36: if (!element) { nit: no curlies around 1-line conditionals https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:36: if (!element) { when should this validly just return? https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:53: keyIndex++) { i don't know if this runs on iOS (or if the linter would like this), but you could do: for (var storedPagesAttribute of storedPagesAttributes) { ... cell.textContent = data[i][storedPagesAttribute]; ... } https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:65: * @param {Object} data An array object representing the request queue Object -> !Array https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:109: * @param {Object} info An object containing both stored pages and it'd be useful to type this more strongly, i.e. @param {{AllPages: !Array, Queue: !Array}} info https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:126: selectedIds.push($(this).val()); $ is not jQuery https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:132: * Initializing everything. is this comment useful? https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:8: #include "base/bind_helpers.h" doesn't look like you need bind_helpers.h https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:25: OfflineInternalsUIMessageHandler(); if this class lives totally in the .cc, you can just inline all the method bodies, i.e. class OfflineInternalsUIMessageHandler : public content::WebUIMessageHandler { public: OfflineInternalsUIMessageHandler() : weak_ptr_factory_(this) {} ~OfflineInternalsUIMessageHandler() override {} void RegisterMessages() override { web_ui()->RegisterMessageCallback(...); ... } https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:59: web_ui()->CallJavascriptFunction("offlineInternals.pagesDeleted"); should this be different from DeleteSelectedPages? https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:66: base::ListValue* queueItems = new base::ListValue(); maybe just inline these for now? i.e. results.Set("AllPages", new base::ListValue()); results.Set("Queue", new base::ListValue()); https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline_internals_ui.h (right): https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.h:11: // The WebUI for chrome://offline-internals nit: end with . https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.h:14: explicit OfflineInternalsUI(content::WebUI* web_ui); you should arguably forward-declare content::WebUI
Thanks for looking at this! https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.css (right): https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.css:3: * found in the LICENSE file. On 2016/05/26 00:54:38, Dan Beam wrote: > nit: * found in the LICENSE file. */ > ^^ Done. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:6: <if expr="is_android"> On 2016/05/26 00:54:38, Dan Beam wrote: > nit: <if> should arguably be 0-indented like #if in C++, as in: > > <if expr="is_android"> > <meta name="viewport" content="width=device-width"> > </if> Is this what you mean? https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:34: <th> </th> On 2016/05/26 00:54:39, Dan Beam wrote: > why is this necessary? Just so the column doesn't completely disappear/collapse when the table has nothing. Arguably, I can use min-width to help with this too https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:8: var storedPagesAttributes = ['onlineUrl', 'namespace', 'size']; On 2016/05/26 00:54:39, Dan Beam wrote: > nit: FINAL_CONST_LIKE_THIS (i.e. STORED_PAGE_ATTRIBUTES) Done. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:25: return; On 2016/05/26 00:54:39, Dan Beam wrote: > shouldn't this blow up because of programmer error when $(tableId) returns null? I've removed the checks. Not sure what you mean by 'blow up'. If we don't do the check, then javascript should just silently log something in console and that'd be it, which is not that much better than currently We shouldn't 'blow up' and render the page unusable. There are a number of other functions coming to the page that will be useful even if someone accidentally deleted a table they shouldn't have. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:36: if (!element) { On 2016/05/26 00:54:39, Dan Beam wrote: > nit: no curlies around 1-line conditionals Done. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:36: if (!element) { On 2016/05/26 00:54:39, Dan Beam wrote: > when should this validly just return? Acknowledged. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:53: keyIndex++) { On 2016/05/26 00:54:39, Dan Beam wrote: > i don't know if this runs on iOS (or if the linter would like this), but you > could do: > > for (var storedPagesAttribute of storedPagesAttributes) { > ... > cell.textContent = data[i][storedPagesAttribute]; > ... > } offline stuff is only available on android. trying to see if the linter likes it. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:65: * @param {Object} data An array object representing the request queue On 2016/05/26 00:54:39, Dan Beam wrote: > Object -> !Array Done. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:109: * @param {Object} info An object containing both stored pages and On 2016/05/26 00:54:39, Dan Beam wrote: > it'd be useful to type this more strongly, i.e. > > @param {{AllPages: !Array, Queue: !Array}} info Done. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:126: selectedIds.push($(this).val()); On 2016/05/26 00:54:39, Dan Beam wrote: > $ is not jQuery Darn :( https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:132: * Initializing everything. On 2016/05/26 00:54:39, Dan Beam wrote: > is this comment useful? Done. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:8: #include "base/bind_helpers.h" On 2016/05/26 00:54:39, Dan Beam wrote: > doesn't look like you need bind_helpers.h Done. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:25: OfflineInternalsUIMessageHandler(); On 2016/05/26 00:54:39, Dan Beam wrote: > if this class lives totally in the .cc, you can just inline all the method > bodies, i.e. > > class OfflineInternalsUIMessageHandler : public content::WebUIMessageHandler { > public: > OfflineInternalsUIMessageHandler() : weak_ptr_factory_(this) {} > ~OfflineInternalsUIMessageHandler() override {} > > void RegisterMessages() override { > web_ui()->RegisterMessageCallback(...); > ... > } These methods are going to get more complicated when the data fetching stuff is added in. The C++ dos and don'ts say that while there are weaker arguments against inlining , the primary concern is readability. I don't really see readability being a big issue here, but I'll defer the answer to you https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:59: web_ui()->CallJavascriptFunction("offlineInternals.pagesDeleted"); On 2016/05/26 00:54:39, Dan Beam wrote: > should this be different from DeleteSelectedPages? It will be in the next iteration, when I add the offline page model in. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:66: base::ListValue* queueItems = new base::ListValue(); On 2016/05/26 00:54:39, Dan Beam wrote: > maybe just inline these for now? i.e. > > results.Set("AllPages", new base::ListValue()); > results.Set("Queue", new base::ListValue()); Done. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline_internals_ui.h (right): https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.h:11: // The WebUI for chrome://offline-internals On 2016/05/26 00:54:39, Dan Beam wrote: > nit: end with . Done. https://codereview.chromium.org/2003883002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.h:14: explicit OfflineInternalsUI(content::WebUI* web_ui); On 2016/05/26 00:54:39, Dan Beam wrote: > you should arguably forward-declare content::WebUI I don't know if that would help anything. I am already inheriting from content::WebUIController, so I need to have the #include anyway. WebUI is in the same .h file, so doesn't seem much of a difference?
lgtm. Very nice!
https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.css (right): https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.css:1: /* Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:2: <html i18n-values="dir:textdirection;lang:language"> when is lang not "en" or dir not "ltr"? https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:6: <if expr="is_android"> when is this false? https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:10: <link rel="stylesheet" href="offline_internals.css"> note: some of these files may be inlined in your current configuration, which may or may not be what you want https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:9: var QUEUE_ATTRIBUTES = ['onlineUrl', 'creation time', 'status']; can you potentially using closure compilation typechecking instead of strings and obj[key] notation? the code, as written, will never be typesafe :( https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:12: * Clear the specified table. @param https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:17: element.textContent = ''; nit: $(tableId).textContent = ''; https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:23: * @param {!Array} data An array object representing stored offline pages nit: end with . https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:27: var row = document.createElement('tr'); indent off https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:33: checkbox.setAttribute('value', data[i]['id']); nit: ['id'] -> .id https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:89: * @param {{AllPages: !Array, Queue:!Array}} info An object containing both Queue: !Array https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:104: var selectedIds = new Array(); new Array() -> [] https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:108: if (box.checked) { no curlies https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:111: } nit: if you're gonna use ES6... var selectedIds = [].filter.call(checkboxes, b => b.checked).map(b => b.id); but general note that ES6 support is bad on iOS and in our presubmit linter https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:113: chrome.send('deleteSelectedPages', [selectedIds]); instead of doing this dance where you say chrome.send('name', params); // export global name // call global name from C++ can you use cr.sendWithPromise() instead? this can handle responses and params and is more testable https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:125: initialize: initialize, why is `initialize` externally exposed?
Thanks for the comments! https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.css (right): https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.css:1: /* Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/05/31 22:00:59, Dan Beam wrote: > no (c) Done. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:2: <html i18n-values="dir:textdirection;lang:language"> On 2016/05/31 22:00:59, Dan Beam wrote: > when is lang not "en" or dir not "ltr"? copy/pasted from another file :/ https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:6: <if expr="is_android"> On 2016/05/31 22:00:59, Dan Beam wrote: > when is this false? Done. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:10: <link rel="stylesheet" href="offline_internals.css"> On 2016/05/31 22:00:59, Dan Beam wrote: > note: some of these files may be inlined in your current configuration, which > may or may not be what you want Acknowledged. I think for now I want them in separate files, to keep track of everything. In later iterations this might change, and I'll bring these inline. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:9: var QUEUE_ATTRIBUTES = ['onlineUrl', 'creation time', 'status']; On 2016/05/31 22:00:59, Dan Beam wrote: > can you potentially using closure compilation typechecking instead of strings > and obj[key] notation? the code, as written, will never be typesafe :( Done. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:12: * Clear the specified table. On 2016/05/31 22:00:59, Dan Beam wrote: > @param Done. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:17: element.textContent = ''; On 2016/05/31 22:00:59, Dan Beam wrote: > nit: $(tableId).textContent = ''; Done. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:23: * @param {!Array} data An array object representing stored offline pages On 2016/05/31 22:00:59, Dan Beam wrote: > nit: end with . Done. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:27: var row = document.createElement('tr'); On 2016/05/31 22:00:59, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:33: checkbox.setAttribute('value', data[i]['id']); On 2016/05/31 22:00:59, Dan Beam wrote: > nit: ['id'] -> .id Done. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:89: * @param {{AllPages: !Array, Queue:!Array}} info An object containing both On 2016/05/31 22:00:59, Dan Beam wrote: > Queue: !Array Done. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:104: var selectedIds = new Array(); On 2016/05/31 22:00:59, Dan Beam wrote: > new Array() -> [] Done. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:108: if (box.checked) { On 2016/05/31 22:00:59, Dan Beam wrote: > no curlies Java-land habit >"< Done https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:111: } On 2016/05/31 22:00:59, Dan Beam wrote: > nit: if you're gonna use ES6... > > var selectedIds = [].filter.call(checkboxes, b => b.checked).map(b => b.id); > > but general note that ES6 support is bad on iOS and in our presubmit linter I don't think this works, because checkboxes is a NodeList, not an Array. Javascript will throw error. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:113: chrome.send('deleteSelectedPages', [selectedIds]); On 2016/05/31 22:00:59, Dan Beam wrote: > instead of doing this dance where you say > > chrome.send('name', params); > > // export global name > // call global name from C++ > > can you use cr.sendWithPromise() instead? this can handle responses and params > and is more testable Done. https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:125: initialize: initialize, On 2016/05/31 22:00:59, Dan Beam wrote: > why is `initialize` externally exposed? Because otherwise I can't seem to call offlineInternals.initialize in line 131...
https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/200001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:111: } > I don't think this works, because checkboxes is a NodeList, not an Array. Javascript will throw error. Drive-by suggestion. If you want to take advantage of the very useful array methods (forEach, map, etc...), you can always convert the NodeList to an array first var array = Array.prototype.slice.call(nodelist); Then you can follow previous suggestion (with or without ES6) of combining filter() and map() calls. var selectedIds = checkboxes.filter(....).map(....);
looks much better https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:21: var OfflinePageItem; i highly recommend you add a compiled_resources2.gyp file in the near future (I don't think these will work as-is when you do that) https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:49: function fillStoredPages(element, data) { nit: data -> pages https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:84: function fillRequestQueue(element, data) { nit: data -> requests https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:34: void DeleteAllPages(const base::ListValue* args); nit: most message handlers are named like: HandleMessageNameFromPage where chrome.send("messageNameFromPage") https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:57: web_ui()->CallJavascriptFunction("cr.webUIResponse", it's better to CallJavascriptFunction() rather than web_ui()->CallJavascriptFunction() the latter is currently being renamed to CallJavascriptFunctionUnsafe() here: https://codereview.chromium.org/1995113002/ https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:76: args->GetString(0, &callback_id); or you could just do: base::Value* callback_id; args->Get(0, &callback_id); https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:80: sprinkle an AllowJavascript(); here
https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:21: var OfflinePageItem; On 2016/06/03 23:27:24, Dan Beam wrote: > i highly recommend you add a compiled_resources2.gyp file in the near future (I > don't think these will work as-is when you do that) If possible, I'd like to push that until the next iteration, as that one will have better story for mapping of the types, etc. https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:49: function fillStoredPages(element, data) { On 2016/06/03 23:27:24, Dan Beam wrote: > nit: data -> pages Done. https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.js:84: function fillRequestQueue(element, data) { On 2016/06/03 23:27:24, Dan Beam wrote: > nit: data -> requests Done. https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:34: void DeleteAllPages(const base::ListValue* args); On 2016/06/03 23:27:25, Dan Beam wrote: > nit: most message handlers are named like: > > HandleMessageNameFromPage > > where chrome.send("messageNameFromPage") Done. https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:57: web_ui()->CallJavascriptFunction("cr.webUIResponse", On 2016/06/03 23:27:24, Dan Beam wrote: > it's better to > > CallJavascriptFunction() > > rather than > > web_ui()->CallJavascriptFunction() > > the latter is currently being renamed to CallJavascriptFunctionUnsafe() here: > https://codereview.chromium.org/1995113002/ Done. https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:76: args->GetString(0, &callback_id); On 2016/06/03 23:27:25, Dan Beam wrote: > or you could just do: > > base::Value* callback_id; > args->Get(0, &callback_id); Done. https://codereview.chromium.org/2003883002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:80: On 2016/06/03 23:27:25, Dan Beam wrote: > sprinkle an > > AllowJavascript(); > > here Done.
lgtm https://codereview.chromium.org/2003883002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2003883002/diff/260001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:2: <html> arguable nit: lang="en" dir="ltr" hardcoded in the HTML (for screenreaders, well... talkback) https://codereview.chromium.org/2003883002/diff/260001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:38: </tbody> nit: <tbody id="stored-pages"></tbody> https://codereview.chromium.org/2003883002/diff/260001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:50: <tbody id="request-queue"> nit: also fits in 1 line https://codereview.chromium.org/2003883002/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2003883002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:57: CallJavascriptFunction("cr.webUIResponse", note: after https://codereview.chromium.org/2040473002/ you should be able to just say: ResolveJavascriptCallback(callback_id, base::StringValue("success"));
Thanks! https://codereview.chromium.org/2003883002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2003883002/diff/260001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:2: <html> On 2016/06/06 23:36:12, Dan Beam wrote: > arguable nit: lang="en" dir="ltr" hardcoded in the HTML (for screenreaders, > well... talkback) Done. https://codereview.chromium.org/2003883002/diff/260001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:38: </tbody> On 2016/06/06 23:36:12, Dan Beam wrote: > nit: <tbody id="stored-pages"></tbody> Done. https://codereview.chromium.org/2003883002/diff/260001/chrome/browser/resourc... chrome/browser/resources/offline_pages/offline_internals.html:50: <tbody id="request-queue"> On 2016/06/06 23:36:12, Dan Beam wrote: > nit: also fits in 1 line Done. https://codereview.chromium.org/2003883002/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2003883002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline_internals_ui.cc:57: CallJavascriptFunction("cr.webUIResponse", On 2016/06/06 23:36:12, Dan Beam wrote: > note: after https://codereview.chromium.org/2040473002/ you should be able to > just say: > > ResolveJavascriptCallback(callback_id, base::StringValue("success")); talked with dpapad@ He will update if mine lands before his. I will update in next iteration if his land before mine.
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org, holte@chromium.org, dewittj@chromium.org, fgorski@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2003883002/#ps300001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2003883002/300001
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Create offline internals page for android BUG=609570 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Offline pages] Create offline internals page for android BUG=609570 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Create offline internals page for android BUG=609570 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [Offline pages] Create offline internals page for android BUG=609570 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b9e16a70339082317d07db9f4165e10257723a28 Cr-Commit-Position: refs/heads/master@{#398394} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/b9e16a70339082317d07db9f4165e10257723a28 Cr-Commit-Position: refs/heads/master@{#398394} |
