[Offline Pages] Connect the offline page logs to the internals page.

[Offline Pages] Connect the offline page logs to the internals page.

BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/e2bfbe7f7650702d48775d11de68e17739334ddd
Cr-Commit-Position: refs/heads/master@{#402363}

[chili, 2016-06-23 05:53:56]

Description was changed from

==========
[Offline Pages] Connect the offline page logs to the internals page.

BUG=609570
==========

to

==========
[Offline Pages] Connect the offline page logs to the internals page.

BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[chili, 2016-06-24 03:35:13]

chili@chromium.org changed reviewers:
+ dewittj@chromium.org, dpapad@chromium.org

[chili, 2016-06-24 03:35:14]

For a screenshot of the UI as it looks with the logs:
https://screenshot.googleplex.com/MY1vH80tSD4

Sorry that it turned out orange. The screenshot tool captured the 'highlight
area to capture' box too :(

Also note that this is dependent on https://codereview.chromium.org/2089413002/

[dewittj, 2016-06-24 17:52:46]

The UI doesn't seem to reflect the enabled/disabled status of the various logs;
can you add that?

[dpapad, 2016-06-24 20:33:43]

https://codereview.chromium.org/2089423002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/offline_pages/offline_internals.js (right):

https://codereview.chromium.org/2089423002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.js:193:
$('log-model-on').onclick = toggleModelOn;
Nit: The four different callback methods can be removed and inlined as follows,

$('log-model-on').onclick =
    browserProxy_.setRecordPageModel.bind(browserProxy_, true);
$('log-model-off').onclick =
    browserProxy_.setRecordPageModel.bind(browserProxy_, false);
$('log-request-on).onclick =
    browserProxy_.setRecordRequestQueue.bind(browserProxy_, true);
$('log-request-off).onclick =
    browserProxy_.setRecordRequestQueue.bind(browserProxy_, false);

https://codereview.chromium.org/2089423002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js
(right):

https://codereview.chromium.org/2089423002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:66: *
@param {boolean} shouldLog True if we should enable logging.
Nit (optional): Avoid using 'we' in comments. "Whether logging should be
enabled."

https://codereview.chromium.org/2089423002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline_internals_ui.cc (right):

https://codereview.chromium.org/2089423002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline_internals_ui.cc:265: args->GetBoolean(0,
&should_record);
CHECK(args->GetBoolean(0, &should_record));

[chili, 2016-06-24 21:43:17]

Re Justin's comment:

I patched the current status in.  However, I don't have time for polish.  For
now, it will load/update the isLogging status only when you press 'Refresh Logs'
or 'Refresh Page'.  Clicking on any of the enable/disable log buttons will cause
the status to be out of date.

I'll polish in a separate CL (after vacation)

https://codereview.chromium.org/2089423002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/offline_pages/offline_internals.js (right):

https://codereview.chromium.org/2089423002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.js:193:
$('log-model-on').onclick = toggleModelOn;
On 2016/06/24 20:33:39, dpapad wrote:
> Nit: The four different callback methods can be removed and inlined as
follows,
> 
> $('log-model-on').onclick =
>     browserProxy_.setRecordPageModel.bind(browserProxy_, true);
> $('log-model-off').onclick =
>     browserProxy_.setRecordPageModel.bind(browserProxy_, false);
> $('log-request-on).onclick =
>     browserProxy_.setRecordRequestQueue.bind(browserProxy_, true);
> $('log-request-off).onclick =
>     browserProxy_.setRecordRequestQueue.bind(browserProxy_, false);

Done.

https://codereview.chromium.org/2089423002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js
(right):

https://codereview.chromium.org/2089423002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:66: *
@param {boolean} shouldLog True if we should enable logging.
On 2016/06/24 20:33:39, dpapad wrote:
> Nit (optional): Avoid using 'we' in comments. "Whether logging should be
> enabled."

Done.

https://codereview.chromium.org/2089423002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline_internals_ui.cc (right):

https://codereview.chromium.org/2089423002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline_internals_ui.cc:265: args->GetBoolean(0,
&should_record);
On 2016/06/24 20:33:39, dpapad wrote:
> CHECK(args->GetBoolean(0, &should_record));

Done.

[dpapad, 2016-06-24 22:14:54]

https://codereview.chromium.org/2089423002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/offline_pages/offline_internals.js (right):

https://codereview.chromium.org/2089423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.js:167:
browserProxy_.getLoggingState().then(logStatus);
logStatus is not a function. Did you mean the following?
browserProxy_.getLoggingState().then(updateLogStatus);


I suggest packaging those request/response functions in a different way, when
there is no good reason to have the response function as a separate top level
function. For example

function updateLogStatus() {
  browserProxy_.getLoggingState().then(function(logStatus) {
    $('model-status').textContent =
        logStatus.modelIsLogging ? 'On' : 'Off';
    $('request-status').textContent =
        logStatus.queueIsLogging ? 'On' : 'Off';
  });
}

https://codereview.chromium.org/2089423002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_model_impl.cc (left):

https://codereview.chromium.org/2089423002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:1050: }
Accidental? I think this is causing the tryjobs to fail.

[chili, 2016-06-24 22:35:23]

https://codereview.chromium.org/2089423002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/offline_pages/offline_internals.js (right):

https://codereview.chromium.org/2089423002/diff/60001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.js:167:
browserProxy_.getLoggingState().then(logStatus);
On 2016/06/24 22:14:53, dpapad wrote:
> logStatus is not a function. Did you mean the following?
> browserProxy_.getLoggingState().then(updateLogStatus);
> 
> 
> I suggest packaging those request/response functions in a different way, when
> there is no good reason to have the response function as a separate top level
> function. For example
> 
> function updateLogStatus() {
>   browserProxy_.getLoggingState().then(function(logStatus) {
>     $('model-status').textContent =
>         logStatus.modelIsLogging ? 'On' : 'Off';
>     $('request-status').textContent =
>         logStatus.queueIsLogging ? 'On' : 'Off';
>   });
> }

Done.

https://codereview.chromium.org/2089423002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_model_impl.cc (left):

https://codereview.chromium.org/2089423002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:1050: }
On 2016/06/24 22:14:53, dpapad wrote:
> Accidental? I think this is causing the tryjobs to fail.

Bad merge :(

[dewittj, 2016-06-24 22:38:27]

lgtm

[dpapad, 2016-06-24 22:52:55]

lgtm

[chili, 2016-06-27 19:03:02]

The CQ bit was checked by chili@chromium.org

[chili, 2016-06-27 19:03:02]

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/2089423002/#ps100001
(title: "upstream test fix and rebase")

[chili, 2016-06-27 19:03:36]

The CQ bit was unchecked by chili@chromium.org

[chili, 2016-06-27 19:07:32]

The CQ bit was checked by chili@chromium.org

[chili, 2016-06-27 19:07:33]

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/2089423002/#ps120001
(title: "rebase update and fix parens")

[commit-bot: I haz the power, 2016-06-27 19:08:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 19:23:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-27 19:23:10]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chili, 2016-06-27 21:30:38]

The CQ bit was checked by chili@chromium.org

[chili, 2016-06-27 21:30:38]

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/2089423002/#ps140001
(title: "add const modifiers")

[commit-bot: I haz the power, 2016-06-27 21:32:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chili, 2016-06-27 21:33:51]

The CQ bit was checked by chili@chromium.org

[chili, 2016-06-27 21:33:52]

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/2089423002/#ps160001
(title: "typo")

[commit-bot: I haz the power, 2016-06-27 21:35:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 21:56:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-27 21:56:41]

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...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chili, 2016-06-27 22:22:15]

The CQ bit was checked by chili@chromium.org

[chili, 2016-06-27 22:22:16]

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/2089423002/#ps180001
(title: "remove unneeded consts")

[commit-bot: I haz the power, 2016-06-27 22:23:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 01:37:53]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-28 01:39:20]

Description was changed from

==========
[Offline Pages] Connect the offline page logs to the internals page.

BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[Offline Pages] Connect the offline page logs to the internals page.

BUG=609570
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/e2bfbe7f7650702d48775d11de68e17739334ddd
Cr-Commit-Position: refs/heads/master@{#402363}
==========

[commit-bot: I haz the power, 2016-06-28 01:39:21]

Patchset 10 (id:??) landed as
https://crrev.com/e2bfbe7f7650702d48775d11de68e17739334ddd
Cr-Commit-Position: refs/heads/master@{#402363}