Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2246563002:
[Offline Pages]Allow multiple urls to be requested in internal page. (Closed)
Description[Offline Pages]Allow multiple urls to be requested in internal page.
Make it possible to make multiple requests in offline internals.
BUG=637394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/9b982065061aec49d78943e817d2c4701e0a2d54
Cr-Commit-Position: refs/heads/master@{#413762}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Parsing on JS side. #
Total comments: 15
Patch Set 3 : Addressed comments. #
Messages
Total messages: 28 (13 generated)
Description was changed from ========== [Offline Pages]Allow multiple urls to be requested in internal page. Make it possible to make multiple requests in offline internals. BUG=637394 ========== to ========== [Offline Pages]Allow multiple urls to be requested in internal page. Make it possible to make multiple requests in offline internals. BUG=637394 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
romax@chromium.org changed reviewers: + chili@chromium.org, michaelpg@chromium.org
PTAL Thanks!
https://codereview.chromium.org/2246563002/diff/1/chrome/browser/resources/of... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2246563002/diff/1/chrome/browser/resources/of... chrome/browser/resources/offline_pages/offline_internals.html:73: Use comma to separate multiple URLs. grammar nit: "Use commas" https://codereview.chromium.org/2246563002/diff/1/chrome/browser/ui/webui/off... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2246563002/diff/1/chrome/browser/ui/webui/off... chrome/browser/ui/webui/offline_internals_ui.cc:9: #include <sstream> alphabetize https://codereview.chromium.org/2246563002/diff/1/chrome/browser/ui/webui/off... chrome/browser/ui/webui/offline_internals_ui.cc:333: std::stringstream ss(request_urls); istringstream? https://codereview.chromium.org/2246563002/diff/1/chrome/browser/ui/webui/off... chrome/browser/ui/webui/offline_internals_ui.cc:335: while (getline(ss, url, ',') && (!url.empty())) { nit: parens not needed around !url.empty() https://codereview.chromium.org/2246563002/diff/1/chrome/browser/ui/webui/off... chrome/browser/ui/webui/offline_internals_ui.cc:340: ResolveJavascriptCallback( it doesn't make sense to repeatedly "resolve" a promise -- perhaps the callback should be called once, representing whether *all* requests succeeded?
https://codereview.chromium.org/2246563002/diff/1/chrome/browser/resources/of... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2246563002/diff/1/chrome/browser/resources/of... chrome/browser/resources/offline_pages/offline_internals.html:73: Use comma to separate multiple URLs. Maybe you feel differently, but I would also put this as the placeholder example <input id="url" type="url" placeholder="www.url1.com, www.url2.com, ..."> https://codereview.chromium.org/2246563002/diff/1/chrome/browser/ui/webui/off... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2246563002/diff/1/chrome/browser/ui/webui/off... chrome/browser/ui/webui/offline_internals_ui.cc:340: ResolveJavascriptCallback( On 2016/08/14 21:31:20, michaelpg wrote: > it doesn't make sense to repeatedly "resolve" a promise -- perhaps the callback > should be called once, representing whether *all* requests succeeded? +1. I believe ResolveJavascriptCallback might also error out if the callback id is resolved multiple times You'd need to change the method signature of the js side to accept multiple status values though Alternatively (may be slower), you can do the url parsing on js side and call into here per url. It may make error messages easier to render.
PTAL sorry for the late reply! I've moved the parsing from c++ to JS. However I was having issues with JS scopes...hope this one not look weird :) Thanks! https://codereview.chromium.org/2246563002/diff/1/chrome/browser/resources/of... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2246563002/diff/1/chrome/browser/resources/of... chrome/browser/resources/offline_pages/offline_internals.html:73: Use comma to separate multiple URLs. On 2016/08/15 18:27:08, chili wrote: > Maybe you feel differently, but I would also put this as the placeholder example > <input id="url" type="url" http://placeholder=%22www.url1.com, http://www.url2.com, ..."> Done. https://codereview.chromium.org/2246563002/diff/1/chrome/browser/ui/webui/off... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2246563002/diff/1/chrome/browser/ui/webui/off... chrome/browser/ui/webui/offline_internals_ui.cc:9: #include <sstream> On 2016/08/14 21:31:20, michaelpg wrote: > alphabetize removed c++ changes and moved to JS side. https://codereview.chromium.org/2246563002/diff/1/chrome/browser/ui/webui/off... chrome/browser/ui/webui/offline_internals_ui.cc:333: std::stringstream ss(request_urls); On 2016/08/14 21:31:19, michaelpg wrote: > istringstream? Done. https://codereview.chromium.org/2246563002/diff/1/chrome/browser/ui/webui/off... chrome/browser/ui/webui/offline_internals_ui.cc:335: while (getline(ss, url, ',') && (!url.empty())) { On 2016/08/14 21:31:20, michaelpg wrote: > nit: parens not needed around !url.empty() Done. https://codereview.chromium.org/2246563002/diff/1/chrome/browser/ui/webui/off... chrome/browser/ui/webui/offline_internals_ui.cc:340: ResolveJavascriptCallback( On 2016/08/15 18:27:08, chili wrote: > On 2016/08/14 21:31:20, michaelpg wrote: > > it doesn't make sense to repeatedly "resolve" a promise -- perhaps the > callback > > should be called once, representing whether *all* requests succeeded? > > +1. I believe ResolveJavascriptCallback might also error out if the callback id > is resolved multiple times > > You'd need to change the method signature of the js side to accept multiple > status values though > Alternatively (may be slower), you can do the url parsing on js side and call > into here per url. It may make error messages easier to render. I think i'm going with parsing the urls in JS side... that seems more reasonable to me..
lgtm with optional comment I don't expect a ton of urls to be entered this way anyway https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:220: browserProxy_.getRequestQueue().then(fillRequestQueue); we're calling .getRequestQueue() a lot... Optional, but one way to fix: add var counter = saveUrls.length; before the for-loop and here, do if (counter == 0) { browserProxy_.getRequestQueue().then(fillRequestQueue); } counter--; since js is single-threaded, shouldn't have any race conditions
https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.html:71: <input id="url" type="url" placeholder="http://www.url1.com, http://www.url2.com, ..."> nit: wrap long lines https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:208: var saveUrl = $('url').value; nit: you don't need this variable https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:213: var it = i; if you use let instead of var in the for loop, i will be scoped to the block like you'd expect in other languages https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:215: .then(function(state) { 4-space indent, while you're here :-) https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:218: saveUrls[it] + ' has been added to queue.\n'; 4-space indent https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:220: browserProxy_.getRequestQueue().then(fillRequestQueue); On 2016/08/19 23:08:49, chili wrote: > we're calling .getRequestQueue() a lot... > > Optional, but one way to fix: > > add var counter = saveUrls.length; before the for-loop > and here, do > if (counter == 0) { > browserProxy_.getRequestQueue().then(fillRequestQueue); > } > counter--; > > since js is single-threaded, shouldn't have any race conditions better, use Promise.all: var promises = []; for (let i = 0; i < saveUrls.length; i++) { promises.push(browserProxy_.addToRequestQueue(saveUrls[i]) .then(.......); } Promise.all(promises).then(function() { browserProxy_.getRequestQueue().then(fillRequestQueue); }); promises.push( https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:223: saveUrls[it] + ' failed to be added to queue.\n'; 4-space indent
comments addressed, sorry but i'm really new to javascript :/ Thanks for the review! https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.html (right): https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.html:71: <input id="url" type="url" placeholder="http://www.url1.com, http://www.url2.com, ..."> On 2016/08/20 00:46:47, michaelpg wrote: > nit: wrap long lines Done. https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:208: var saveUrl = $('url').value; On 2016/08/20 00:46:47, michaelpg wrote: > nit: you don't need this variable Done. https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:213: var it = i; On 2016/08/20 00:46:47, michaelpg wrote: > if you use let instead of var in the for loop, i will be scoped to the block > like you'd expect in other languages Done. https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:215: .then(function(state) { On 2016/08/20 00:46:47, michaelpg wrote: > 4-space indent, while you're here :-) Done. https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:218: saveUrls[it] + ' has been added to queue.\n'; On 2016/08/20 00:46:47, michaelpg wrote: > 4-space indent Done. https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:220: browserProxy_.getRequestQueue().then(fillRequestQueue); On 2016/08/19 23:08:49, chili wrote: > we're calling .getRequestQueue() a lot... > > Optional, but one way to fix: > > add var counter = saveUrls.length; before the for-loop > and here, do > if (counter == 0) { > browserProxy_.getRequestQueue().then(fillRequestQueue); > } > counter--; > > since js is single-threaded, shouldn't have any race conditions Done. https://codereview.chromium.org/2246563002/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:223: saveUrls[it] + ' failed to be added to queue.\n'; On 2016/08/20 00:46:47, michaelpg wrote: > 4-space indent Done.
lgtm
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chili@chromium.org Link to the patchset: https://codereview.chromium.org/2246563002/#ps40001 (title: "Addressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by romax@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by romax@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages]Allow multiple urls to be requested in internal page. Make it possible to make multiple requests in offline internals. BUG=637394 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Offline Pages]Allow multiple urls to be requested in internal page. Make it possible to make multiple requests in offline internals. BUG=637394 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/9b982065061aec49d78943e817d2c4701e0a2d54 Cr-Commit-Position: refs/heads/master@{#413762} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9b982065061aec49d78943e817d2c4701e0a2d54 Cr-Commit-Position: refs/heads/master@{#413762} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
