Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2172443004:
[Offline Pages] Add network status, save request textbox, and offline url link to offline internals… (Closed)
Description[Offline Pages] Add network status, save request textbox, and offline url link to offline internals page
BUG=629880, 629879
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/de0afdc23dbee04f1d2236245ec0ebb7d3b615eb
Cr-Commit-Position: refs/heads/master@{#407332}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 14
Patch Set 3 : Resolve code review comments #
Total comments: 1
Patch Set 4 : Remove to_string and update css #Patch Set 5 : Update SavePageRequest call after rebase #
Messages
Total messages: 30 (18 generated)
Description was changed from ========== [Offline Pages] Add network status, save request textbox, and offline url link to offline internals page BUG=629880,629879 ========== to ========== [Offline Pages] Add network status, save request textbox, and offline url link to offline internals page BUG=629880,629879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by chili@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [Offline Pages] Add network status, save request textbox, and offline url link to offline internals page BUG=629880,629879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Offline Pages] Add network status, save request textbox, and offline url link to offline internals page BUG=629880,629879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
chili@chromium.org changed reviewers: + dewittj@chromium.org, dpapad@chromium.org
The CQ bit was checked by chili@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...
https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:214: $('url').textContent = ''; Is this equivalent to $('url').value = ''; If so let's use |value| instead of |textContent| for consistency with line 208. https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js (right): https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:102: * @return {!Promise<boolean>} A promise firing after added to queue. What does the boolean indicate? Please explain in the comment. https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:108: * @return {!Promise<!string>} A promise firing when the network status "!string" is same as "string", since primitive types are considered non-nullable by default by the Closule compiler. We usually omit the "!" in such cases. https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:34: class OfflineInternalsUIMessageHandler : public content::WebUIMessageHandler { Optional suggestion for follow up cleanup CL. - Move OfflineInternalsUIMessageHandler and OfflineInternalsUI to their own file. - Move all offline internals related code under chrome/browser/ui/webui/offline_internals/ https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:286: if (net::NetworkChangeNotifier::IsOffline()) { Nit: I think you can compress this by using the ternary operator. ResolveJavascriptCallback( *callback_id, base::StringValue(net::NetworkChangeNotifier::IsOffline() ? "Offline" : "Online")); https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:341: offline_pages::ClientId(offline_pages::kAsyncNamespace, "foo")); It is not clear here why "foo" is hardcoded. Please add a comment explaining why this parameter does not matter.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for taking a look! https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.js (right): https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.js:214: $('url').textContent = ''; On 2016/07/21 18:57:25, dpapad wrote: > Is this equivalent to > $('url').value = ''; > > If so let's use |value| instead of |textContent| for consistency with line 208. Done. https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js (right): https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:102: * @return {!Promise<boolean>} A promise firing after added to queue. On 2016/07/21 18:57:25, dpapad wrote: > What does the boolean indicate? Please explain in the comment. Done. https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:108: * @return {!Promise<!string>} A promise firing when the network status On 2016/07/21 18:57:25, dpapad wrote: > "!string" is same as "string", since primitive types are considered non-nullable > by default by the Closule compiler. We usually omit the "!" in such cases. Done. https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:34: class OfflineInternalsUIMessageHandler : public content::WebUIMessageHandler { On 2016/07/21 18:57:25, dpapad wrote: > Optional suggestion for follow up cleanup CL. > - Move OfflineInternalsUIMessageHandler and OfflineInternalsUI to their own > file. > - Move all offline internals related code under > chrome/browser/ui/webui/offline_internals/ Added TODO. Ack. (There's no 'internal cleanup' bug type in chromium bug tracker :/) https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:286: if (net::NetworkChangeNotifier::IsOffline()) { On 2016/07/21 18:57:25, dpapad wrote: > Nit: I think you can compress this by using the ternary operator. > > ResolveJavascriptCallback( > *callback_id, > base::StringValue(net::NetworkChangeNotifier::IsOffline() ? "Offline" : > "Online")); Done. https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:341: offline_pages::ClientId(offline_pages::kAsyncNamespace, "foo")); On 2016/07/21 18:57:25, dpapad wrote: > It is not clear here why "foo" is hardcoded. Please add a comment explaining why > this parameter does not matter. I changed this in a following version but must've committed after I ran cl upload. This is supposed to be the string version of a random number.
https://codereview.chromium.org/2172443004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/offline_internals_ui.cc (right): https://codereview.chromium.org/2172443004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/offline_internals_ui.cc:345: std::to_string(std::rand()))))); std::to_string() is not currently allowed in the Chromium codebase, see https://chromium-cpp.appspot.com/ .
lgtm once the std::to_string is removed. https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.css (right): https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.css:51: display: inline-block; Any particular reason for this? Perhaps it should be a span if you want it to be inline.
I am actually a little confused on https://chromium-cpp.appspot.com/ The line that contains std::to_string explicitly calls out all the string to number methods, which to_string is not... I have replaced its use with stringstream https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... File chrome/browser/resources/offline_pages/offline_internals.css (right): https://codereview.chromium.org/2172443004/diff/20001/chrome/browser/resource... chrome/browser/resources/offline_pages/offline_internals.css:51: display: inline-block; On 2016/07/22 17:52:03, dewittj wrote: > Any particular reason for this? Perhaps it should be a span if you want it to > be inline. I was messing around with layout. fixed
lgtm
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2172443004/#ps60001 (title: "Remove to_string and update css")
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2172443004/#ps80001 (title: "Update SavePageRequest call after rebase")
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.
Description was changed from ========== [Offline Pages] Add network status, save request textbox, and offline url link to offline internals page BUG=629880,629879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Offline Pages] Add network status, save request textbox, and offline url link to offline internals page BUG=629880,629879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Add network status, save request textbox, and offline url link to offline internals page BUG=629880,629879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Offline Pages] Add network status, save request textbox, and offline url link to offline internals page BUG=629880,629879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/de0afdc23dbee04f1d2236245ec0ebb7d3b615eb Cr-Commit-Position: refs/heads/master@{#407332} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/de0afdc23dbee04f1d2236245ec0ebb7d3b615eb Cr-Commit-Position: refs/heads/master@{#407332} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
