[Offline Pages] Link the internals page with the offline model and request

chrome/browser/resources/offline_pages/offline_internals.js

Index: chrome/browser/resources/offline_pages/offline_internals.js
diff --git a/chrome/browser/resources/offline_pages/offline_internals.js b/chrome/browser/resources/offline_pages/offline_internals.js
index d5fb091e9a3315cc5aaf66c1d61079b5cfcc50bb..59d3735444d26bc7857295ef206ce0900bff74f3 100644
--- a/chrome/browser/resources/offline_pages/offline_internals.js
+++ b/chrome/browser/resources/offline_pages/offline_internals.js
@@ -2,110 +2,114 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+/**
+ * @typedef {{
+ *   onlineUrl: string,
+ *   creationTime: number,
+ *   id: string,
+ *   namespace: string,
+ *   size: string,
+ *   filePath: string,
+ *   lastAccessTime: number,
+ *   accessCount: number
+ *  }}

[Dan Beam, 2016/06/09 23:35:01]

indent off by 1 \s

[chili, 2016/06/10 01:04:51]

Done.

+ */
+var OfflinePageItem;
+
+/**
+ * @typedef {{
+ *   status: string,
+ *   onlineUrl: string,
+ *   creationTime: number,
+ *   id: string,
+ *   namespace: string,
+ *   lastAttempt: number
+ * }}
+ */
+var SavePageRequest;
+
 cr.define('offlineInternals', function() {
   'use strict';
 
-  /**
-   * @typedef {{
-   *   onlineUrl: string,
-   *   creationTime: number,
-   *   status: number,
-   *   id: string,
-   *   namespace: string,
-   *   size: string,
-   *   filePath: string,
-   *   lastAccessTime: number,
-   *   accessCount: number
-   *  }}
-   */
-  var OfflinePageItem;
+  /** @type {Array<OfflinePageItem>} */

[Dan Beam, 2016/06/09 23:35:01]

nit: ?Array<!OfflinePageItems>

! means "not null"
? means "or null"

the absence of "?" or "!" means "or null but the reader isn't very sure" ;)

[dewittj, 2016/06/09 23:41:25]

I don't think "!" is required for record types (which OfflinePageItem is).

[Dan Beam, 2016/06/09 23:50:01]

@typedefs are nullable by default

[dewittj, 2016/06/10 00:13:49]

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

[Dan Beam, 2016/06/10 00:28:17]

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

[dpapad, 2016/06/10 00:47:04]

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.

[chili, 2016/06/10 01:04:51]

Acknowledged.

[chili, 2016/06/10 01:04:51]

Acknowledged.

[chili, 2016/06/10 01:04:51]

Done.

[chili, 2016/06/10 20:21:03]

removed the !

+  var currentStoredPagesData = null;
 
-  /**
-   * @typedef {{
-   *   status: string,
-   *   onlineUrl: string,
-   *   creationTime: number,
-   *   id: string,
-   *   namespace: string,
-   *   attemptCount: number
-   * }}
-   */
-  var SavePageRequest;
-
-  /**
-   * Clear the specified table.
-   * @param {string} tableId id of the table to clear.
-   */
-  function clearTable(tableId) {
-    $(tableId).textContent = '';
-  }
+  /** @type {Array<SavePageRequest>} */
+  var currentRequestQueueData = null;
 
   /**
    * Fill stored pages table.
-   * @param {HTMLElement} element A HTML element.
-   * @param {!Array<OfflinePageItem>} pages An array object representing
+   * @param {!Array<OfflinePageItem>} data An array object representing

[Dan Beam, 2016/06/09 23:35:01]

unless this array can have null in it, !Array<!OfflinePageItem>
                                              ^

[Dan Beam, 2016/06/09 23:35:01]

why did you revert pages to data?

[chili, 2016/06/10 01:04:51]

Done.

[chili, 2016/06/10 01:04:51]

I had this CL sitting for almost as long as the previous one.  The merge didn't
go correctly

    *     stored offline pages.
    */
-  function fillStoredPages(element, pages) {
-    for (var i = 0; i < pages.length; i++) {
+  function fillStoredPages(data) {
+    var element = $('stored-pages');

[Dan Beam, 2016/06/09 23:35:01]

element -> storedPages?

[chili, 2016/06/10 01:04:51]

Done.

+    element.textContent = '';
+
+    for (var i = 0; i < data.length; i++) {
       var row = document.createElement('tr');
 
       var checkboxCell = document.createElement('td');
       var checkbox = document.createElement('input');
       checkbox.setAttribute('type', 'checkbox');
       checkbox.setAttribute('name', 'stored');
-      checkbox.setAttribute('value', pages[i].id);
+      checkbox.setAttribute('value', data[i].id);
 
       checkboxCell.appendChild(checkbox);
       row.appendChild(checkboxCell);
 
       var cell = document.createElement('td');
-      cell.textContent = pages[i].onlineUrl;
+      cell.textContent = data[i].onlineUrl;
       row.appendChild(cell);
 
       cell = document.createElement('td');
-      cell.textContent = pages[i].namespace;
+      cell.textContent = data[i].namespace;
       row.appendChild(cell);
 
       cell = document.createElement('td');
-      cell.textContent = pages[i].size;
+      cell.textContent = Math.round(data[i].size / 1024);
       row.appendChild(cell);
 
       element.appendChild(row);
     }
+    currentStoredPagesData = data;
   }
 
   /**
    * Fill requests table.
-   * @param {HTMLElement} element A HTML element.
-   * @param {!Array<SavePageRequest>} requests An array object representing
+   * @param {!Array<SavePageRequest>} data An array object representing
    *     the request queue.
    */
-  function fillRequestQueue(element, requests) {
-    for (var i = 0; i < requests.length; i++) {
+  function fillRequestQueue(data) {

[Dan Beam, 2016/06/09 23:35:01]

same question regarding data vs requests

[chili, 2016/06/10 01:04:51]

Same as above... bad merge 

+    var element = $('request-queue');
+    element.textContent = '';
+
+    for (var i = 0; i < data.length; i++) {
       var row = document.createElement('tr');
 
       var cell = document.createElement('td');
-      cell.textContent = requests[i].onlineUrl;
+      cell.textContent = data[i].onlineUrl;
       row.appendChild(cell);
 
       cell = document.createElement('td');
-      cell.textContent = new Date(requests[i].creationTime);
+      cell.textContent = new Date(data[i].creationTime);
       row.appendChild(cell);
 
       cell = document.createElement('td');
-      cell.textContent = requests[i].status;
+      cell.textContent = data[i].status;
       row.appendChild(cell);
 
       element.appendChild(row);
     }
+    currentRequestQueueData = data;

[Dan Beam, 2016/06/09 23:35:01]

currentRequests?

[chili, 2016/06/10 01:04:51]

Done.

   }
 
   /**
    * Refresh all displayed information.
    */
   function refreshAll() {
-    cr.sendWithPromise('getOfflineInternalsInfo').then(setOfflineInternalsInfo);
+    cr.sendWithPromise('getStoredPagesInfo').then(fillStoredPages);
+    cr.sendWithPromise('getRequestQueueInfo').then(fillRequestQueue);
   }
 
   /**
@@ -117,25 +121,66 @@ cr.define('offlineInternals', function() {
 
   /**
    * Callback when pages are deleted.
-   * @param {string} deletePageStatus The status of delete page call.
+   * @param {string} status The status of the request.
    */
-  function pagesDeleted(deletePageStatus) {
-    // TODO(chili): decide what to do here. Perhaps a refresh of just
-    // the stored pages table?
+  function pagesDeleted(status) {
+    $('page-actions-info').textContent = status;
+    cr.sendWithPromise('getStoredPagesInfo').then(fillStoredPages);
   }
 
   /**
-   * Callback when information is loaded.
-   * @param {{AllPages: !Array<OfflinePageItem>,
-   *          Queue: !Array<SavePageRequest>}} info An object containing both
-   *     stored pages and request queue status.
+   * Helper function to JSON-escape and add quotes around a string.
+   * @param {string} strObj The obj to escape and add quotes around.
+   * @return {string} the escaped string.

[dewittj, 2016/06/09 23:41:25]

nit: capitalize the T

[chili, 2016/06/10 01:04:51]

Done.

    */
-  function setOfflineInternalsInfo(info) {
-    clearTable('stored-pages');
-    clearTable('request-queue');
+  function escapeString(strObj) {
+    strObj.replace(/"/g, '""'); // CSV single quotes are encoded as "".

[Dan Beam, 2016/06/09 23:35:01]

this returns a new string and does not work in place (i.e. it currently does
nothing)

[chili, 2016/06/10 01:04:51]

oops. fixed.

+    return '"' + strObj + '"'; // There can be commas in the string.

[Dan Beam, 2016/06/09 23:35:01]

return '"' + strObj.replace(/"/g, '""') + '"';

[chili, 2016/06/10 01:04:51]

Done.

+  }
 
-    fillStoredPages($('stored-pages'), info.AllPages);
-    fillRequestQueue($('request-queue'), info.Queue);
+  /**
+   * Downloads all the stored page and request queue information into a file.
+   */
+  function download() {
+    var csv = '';
+    // Create header & csv for stored pages.
+    if (currentStoredPagesData && currentStoredPagesData.length > 0) {
+      csv += 'Online URL,Namespace,Size,ID,File Path,Creation Time,' +
+          'Last Accessed Time,Access Count\n';
+      for (let obj of currentStoredPagesData) {
+        var objFieldArr = [
+            obj.onlineUrl,
+            obj.namespace,
+            obj.size,
+            obj.id,
+            obj.filePath,
+            new Date(obj.creationTime).toString(),
+            new Date(obj.lastAccessTime).toString(),
+            obj.accessCount];
+        objFieldArr = objFieldArr.map(escapeString);
+        csv += objFieldArr.join(',') + '\n';
+      }
+    }
+    csv += '\n';
+    // Create header & csv for request queue.
+    if (currentRequestQueueData && currentRequestQueueData.length > 0) {
+      csv += 'Online URL,Creation Time,Status,Namespace,Last Attempt Time,ID\n';
+
+      for (let obj of currentRequestQueueData) {
+        var objFieldArr = [
+          obj.onlineUrl,
+          new Date(obj.creationTime).toString(),
+          obj.status,
+          obj.namespace,
+          new Date(obj.lastAttempt).toString(),
+          obj.id];
+        objFieldArr = objFieldArr.map(escapeString);
+        csv += objFieldArr.join(',') + '\n';
+      }
+    }
+
+    var uriContent = 'data:text/csv,' + encodeURIComponent(csv);
+    window.open(uriContent, 'dump.csv');
   }

[Dan Beam, 2016/06/09 23:35:01]

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.)?

[chili, 2016/06/10 01:04:51]

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.

[Dan Beam, 2016/06/13 19:13:28]

this doesn't make much sense.  why would you write a CSV generator just to
remove it and use the pre-written one?

[chili, 2016/06/13 20:47:14]

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

[Dan Beam, 2016/06/13 21:13:23]

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?
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...

[chili, 2016/06/13 23:39:17]

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.

 
   /**
@@ -144,9 +189,11 @@ cr.define('offlineInternals', function() {
   function deleteSelectedPages() {
     var checkboxes = document.getElementsByName('stored');
     var selectedIds = [];
-    for (var checkbox of checkboxes) {
-      if (checkbox.checked)
-        selectedIds.push(checkbox.value);
+
+    for (var i = 0; i < checkboxes.length; i++) {
+      if (checkboxes[i].checked) {

[Dan Beam, 2016/06/09 23:35:01]

no curlies

[chili, 2016/06/10 01:04:51]

Done.

+        selectedIds.push(checkboxes[i].value);
+      }
     }
 
     cr.sendWithPromise('deleteSelectedPages', selectedIds).then(pagesDeleted);
@@ -156,6 +203,7 @@ cr.define('offlineInternals', function() {
     $('clear-all').onclick = deleteAllPages;
     $('clear-selected').onclick = deleteSelectedPages;
     $('refresh').onclick = refreshAll;
+    $('download').onclick = download;
     refreshAll();
   }