Return operation name in prefetch request callback and add internal page hookup

Return operation name in prefetch request callback and add internal page hookup

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2928243002
Cr-Commit-Position: refs/heads/master@{#479499}
Committed: https://chromium.googlesource.com/chromium/src/+/704f4daf9c7a181b58c9d4b248dd9254baa2b4fd

[jianli, 2017-06-10 00:45:13]

Description was changed from

==========
Return operation name in prefetch request callback and add internal page hookup

BUG=none
==========

to

==========
Return operation name in prefetch request callback and add internal page hookup

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[jianli, 2017-06-10 00:45:19]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-10 00:45:42]

Dry run: CQ is trying da patch.

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

[jianli, 2017-06-10 00:48:59]

jianli@chromium.org changed reviewers:
+ dewittj@chromium.org

[commit-bot: I haz the power, 2017-06-10 00:56:53]

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

[commit-bot: I haz the power, 2017-06-10 00:56:54]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[dewittj, 2017-06-12 18:31:04]

in general this lgtm, with some concernsa bout the internals UI

https://codereview.chromium.org/2928243002/diff/1/chrome/browser/resources/of...
File chrome/browser/resources/offline_pages/offline_internals.html (right):

https://codereview.chromium.org/2928243002/diff/1/chrome/browser/resources/of...
chrome/browser/resources/offline_pages/offline_internals.html:90: <input
id="request-data" type="text">
There should be different input boxes for the two types of operation, and at
least a tiny bit of info about what to put here.

GeneratePageBundle: "A list of URLs, separated by spaces"
GetOperation: "An operation name returned by GeneratePageBundle"

I'd also like to see more structured output for GeneratePageBundle, something
readable on mobile.  That can be left for a future change, but please add an
Asana task or a bug for it.

https://codereview.chromium.org/2928243002/diff/1/chrome/browser/resources/of...
chrome/browser/resources/offline_pages/offline_internals.html:96: <div
id="prefetch-actions-info"></div>
This should probably have different formatting that preserves whitespace.  maybe
style="white-space: pre-wrap"?

https://codereview.chromium.org/2928243002/diff/1/chrome/browser/ui/webui/off...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2928243002/diff/1/chrome/browser/ui/webui/off...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:86:
std::string GetStringRenderPageInfoList(
Is it possible to just use text_format.h?

https://cs.chromium.org/chromium/src/third_party/protobuf/src/google/protobuf...

[jianli, 2017-06-12 22:40:30]

https://codereview.chromium.org/2928243002/diff/1/chrome/browser/resources/of...
File chrome/browser/resources/offline_pages/offline_internals.html (right):

https://codereview.chromium.org/2928243002/diff/1/chrome/browser/resources/of...
chrome/browser/resources/offline_pages/offline_internals.html:90: <input
id="request-data" type="text">
On 2017/06/12 18:31:04, dewittj wrote:
> There should be different input boxes for the two types of operation, and at
> least a tiny bit of info about what to put here.
> 
> GeneratePageBundle: "A list of URLs, separated by spaces"
> GetOperation: "An operation name returned by GeneratePageBundle"
> 
> I'd also like to see more structured output for GeneratePageBundle, something
> readable on mobile.  That can be left for a future change, but please add an
> Asana task or a bug for it.

Added 2nd input box. Added explanation for input box values. Also made the
output more structured.

https://codereview.chromium.org/2928243002/diff/1/chrome/browser/resources/of...
chrome/browser/resources/offline_pages/offline_internals.html:96: <div
id="prefetch-actions-info"></div>
On 2017/06/12 18:31:04, dewittj wrote:
> This should probably have different formatting that preserves whitespace. 
maybe
> style="white-space: pre-wrap"?

Done.

https://codereview.chromium.org/2928243002/diff/1/chrome/browser/ui/webui/off...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2928243002/diff/1/chrome/browser/ui/webui/off...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:86:
std::string GetStringRenderPageInfoList(
On 2017/06/12 18:31:04, dewittj wrote:
> Is it possible to just use text_format.h?
> 
>
https://cs.chromium.org/chromium/src/third_party/protobuf/src/google/protobuf...

We can't and don't want to use it because:
* text_format is only included in protobuf_full version, not protobuf_lite which
is linked into Chromium.
* RenderPageInfo is converted from proto data so we can't use text_format.

[jianli, 2017-06-12 22:43:28]

jianli@chromium.org changed reviewers:
+ dpapad@chromium.org

[jianli, 2017-06-12 22:43:28]

dpapad for webui

[jianli, 2017-06-12 22:43:38]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 22:44:14]

Dry run: 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, 2017-06-12 22:46:50]

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

[commit-bot: I haz the power, 2017-06-12 22:46:50]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jianli, 2017-06-12 23:20:23]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 23:20:52]

Dry run: CQ is trying da patch.

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

[dpapad, 2017-06-12 23:29:20]

https://codereview.chromium.org/2928243002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h
(right):

https://codereview.chromium.org/2928243002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:71: //
Sends a request to genrate page bundle.
The comment sounds a bit confusing. Does calling "HandleGeneratePageBundle" send
a request, or handle a request that is sent from JS? Some question for
"HandleGetOperation" below.

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

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.js:183: * @param
{!IsLogging} logStatus Status of logging.
I don't think this passes compilation. The parameter |logStatus| mismaches the
actual param |info| below. Also |textContent| expects a string value, not an
IsLogging object.

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

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:137: *
@return {!Promise} A promise firing when the request is sent.
Can you document the returned data structure? Promise<SomeType> where SomeType
is a typedef defined in this file (or string,number etc).

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:143: *
@return {!Promise} A promise firing when the request is sent.
Same here.

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:93:
const std::vector<offline_pages::RenderPageInfo>& pages) {
Why is this function generating a stringified version of the data structure to
be passed to JS? Can you just pass a plain object, that comes out as a normal JS
object on the other side?

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:358:
base::Value(GetStringFromPrefetchRequestStatus(status) + "\n" +
Same question. Why are we "encoding" the result in a custom formatted string?
Can we just pass back an object, and let the JS side handle the formatting?

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h
(right):

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:79: //
Sends a request to genrate page bundle.
The comment sounds a bit confusing. Does calling "HandleGeneratePageBundle" send
a request, or handle a request that is sent from JS? Some question for
"HandleGetOperation" below.

[commit-bot: I haz the power, 2017-06-12 23:37:19]

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

[commit-bot: I haz the power, 2017-06-12 23:37:20]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[jianli, 2017-06-12 23:48:14]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[jianli, 2017-06-12 23:48:15]

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

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.js:183: * @param
{!IsLogging} logStatus Status of logging.
On 2017/06/12 23:29:20, dpapad wrote:
> I don't think this passes compilation. The parameter |logStatus| mismaches the
> actual param |info| below. Also |textContent| expects a string value, not an
> IsLogging object.

Done.

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

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:137: *
@return {!Promise} A promise firing when the request is sent.
On 2017/06/12 23:29:20, dpapad wrote:
> Can you document the returned data structure? Promise<SomeType> where SomeType
> is a typedef defined in this file (or string,number etc).

Done.

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:143: *
@return {!Promise} A promise firing when the request is sent.
On 2017/06/12 23:29:20, dpapad wrote:
> Same here.

Done.

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:93:
const std::vector<offline_pages::RenderPageInfo>& pages) {
On 2017/06/12 23:29:20, dpapad wrote:
> Why is this function generating a stringified version of the data structure to
> be passed to JS? Can you just pass a plain object, that comes out as a normal
JS
> object on the other side?

I don't think JS needs to know anything about the object. Here we only want to
dump the result.

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:358:
base::Value(GetStringFromPrefetchRequestStatus(status) + "\n" +
On 2017/06/12 23:29:20, dpapad wrote:
> Same question. Why are we "encoding" the result in a custom formatted string?
> Can we just pass back an object, and let the JS side handle the formatting?

I think we only want to dump the result for debugging purpose. We don't want to
complicate the JS code to know about all the details.

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h
(right):

https://codereview.chromium.org/2928243002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:79: //
Sends a request to genrate page bundle.
On 2017/06/12 23:29:20, dpapad wrote:
> The comment sounds a bit confusing. Does calling "HandleGeneratePageBundle"
send
> a request, or handle a request that is sent from JS? Some question for
> "HandleGetOperation" below.

This will trigger sending a network request and handling its response. Updated
the comment to make it better.

[commit-bot: I haz the power, 2017-06-12 23:48:40]

Dry run: 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, 2017-06-13 00:03:16]

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

[commit-bot: I haz the power, 2017-06-13 00:03:17]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[jianli, 2017-06-13 00:17:20]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 00:17:48]

Dry run: 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, 2017-06-13 00:35:58]

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

[commit-bot: I haz the power, 2017-06-13 00:35:59]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dpapad, 2017-06-13 01:14:26]

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

https://codereview.chromium.org/2928243002/diff/80001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.js:183: * @param {info}
The result of performing the prefetch actions.
See
https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the....

@param {string} info The result of performing the prefetch actions.

Please make sure that the closure compilation succeeds before sending for
review, (current failure is shown at
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...).

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

https://codereview.chromium.org/2928243002/diff/80001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:140:
generatePageBundle: function(data) {},
The returned type should be reported inside the {!Promise<string>} annotation.
You don't need to any of the comment 

/**
  * Request generating a page bundle.
  * @return {!Promise<string>} A string describing the result.
  */

Also @param annotation is missing.

https://codereview.chromium.org/2928243002/diff/80001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:144: *
@return {!Promise} A promise firing when the request is sent. Promise
Same here.

[jianli, 2017-06-13 20:41:00]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 20:41:40]

Dry run: CQ is trying da patch.

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

[jianli, 2017-06-13 22:06:13]

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

https://codereview.chromium.org/2928243002/diff/80001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.js:183: * @param {info}
The result of performing the prefetch actions.
On 2017/06/13 01:14:26, dpapad wrote:
> See
>
https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the....
> 
> @param {string} info The result of performing the prefetch actions.
> 
> Please make sure that the closure compilation succeeds before sending for
> review, (current failure is shown at
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...).

Done.

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

https://codereview.chromium.org/2928243002/diff/80001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:140:
generatePageBundle: function(data) {},
On 2017/06/13 01:14:26, dpapad wrote:
> The returned type should be reported inside the {!Promise<string>} annotation.
> You don't need to any of the comment 
> 
> /**
>   * Request generating a page bundle.
>   * @return {!Promise<string>} A string describing the result.
>   */
> 
> Also @param annotation is missing.

Done.

https://codereview.chromium.org/2928243002/diff/80001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals_browser_proxy.js:144: *
@return {!Promise} A promise firing when the request is sent. Promise
On 2017/06/13 01:14:26, dpapad wrote:
> Same here.

Done.

[commit-bot: I haz the power, 2017-06-13 22:17:50]

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

[commit-bot: I haz the power, 2017-06-13 22:17:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2017-06-14 00:52:27]

webui LGTM

https://codereview.chromium.org/2928243002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/offline_pages/offline_internals.js (right):

https://codereview.chromium.org/2928243002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/offline_pages/offline_internals.js:183: @param {string}
info The result of performing the prefetch actions.
Nit: JSDoc comment indentation is off (and asterisk is missing).

/**
 * Callback for prefetch actions.
 * @param {string} info The result of performing the prefetch actions.
 */

[jianli, 2017-06-14 19:14:55]

https://codereview.chromium.org/2928243002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/offline_pages/offline_internals.js (right):

https://codereview.chromium.org/2928243002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/offline_pages/offline_internals.js:183: @param {string}
info The result of performing the prefetch actions.
On 2017/06/14 00:52:26, dpapad wrote:
> Nit: JSDoc comment indentation is off (and asterisk is missing).
> 
> /**
>  * Callback for prefetch actions.
>  * @param {string} info The result of performing the prefetch actions.
>  */

Done.

[jianli, 2017-06-14 19:15:04]

The CQ bit was checked by jianli@chromium.org

[jianli, 2017-06-14 19:15:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dewittj@chromium.org, dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2928243002/#ps120001
(title: "Fix comment indent in JS")

[commit-bot: I haz the power, 2017-06-14 19:15:41]

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, 2017-06-14 20:52:15]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497467704422420,
"parent_rev": "e17e32d3fba9614e1dc54ffd21aa3ae56409a9f6", "commit_rev":
"704f4daf9c7a181b58c9d4b248dd9254baa2b4fd"}

[commit-bot: I haz the power, 2017-06-14 20:52:28]

Description was changed from

==========
Return operation name in prefetch request callback and add internal page hookup

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Return operation name in prefetch request callback and add internal page hookup

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2928243002
Cr-Commit-Position: refs/heads/master@{#479499}
Committed:
https://chromium.googlesource.com/chromium/src/+/704f4daf9c7a181b58c9d4b248dd...
==========

[commit-bot: I haz the power, 2017-06-14 20:52:32]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/704f4daf9c7a181b58c9d4b248dd...