Adds a delete button to chrome:offline-internals

Adds a delete button to chrome:offline-internals

We currently don't have a way to delete 'stuck' requests to get back
to a clean state, or to remove requests that are in the way of testing.
This new button will allow us to do just that.

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

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

[Pete Williamson, 2016-09-09 23:46:41]

Description was changed from

==========
Adds a delete button to chrome:offline-internals

We currently don't have a way to delete 'stuck' requests to get back
to a clean state, or to remove requests that are in the way of testing.
This new button will allow us to do just that.

BUG=654684
==========

to

==========
Adds a delete button to chrome:offline-internals

We currently don't have a way to delete 'stuck' requests to get back
to a clean state, or to remove requests that are in the way of testing.
This new button will allow us to do just that.

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

[Pete Williamson, 2016-09-12 18:32:34]

petewil@chromium.org changed reviewers:
+ chili@chromium.org

[Pete Williamson, 2016-09-12 18:32:35]

This adds a "Delete" and a "Delete All" button for SavePageRequests.  I followed
the same checkbox pattern used for removing OfflinePageItems.

[chili, 2016-09-12 20:26:02]

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

https://codereview.chromium.org/2328973003/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.html:62: <button
id="delete-all">Delete all</button>
nit: can we id this to delete-all-requests and delete-selected-requests?   This
will help with any name confusion with clear stored pages vs delete here (I'll
make a note to rename the cleared pages ids too)

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

https://codereview.chromium.org/2328973003/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.js:154:
$('page-actions-info').textContent = status;
page-actions-info is a div between the stored pages table and request queue
table.  Should we have another info? (i.e. have stored-pages-actions-info and
request-queue-actions-info)?

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

https://codereview.chromium.org/2328973003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:60:
return "Store failure";
should we have slightly better messaging for which request failed to delete?

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

https://codereview.chromium.org/2328973003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:64: void
DeleteAllGetCallback(
nit: this method name is really confusing...  what about...
"HandleGetAllRequestsForDelete"? or "OnGetAllRequestsForDelete"?

[Pete Williamson, 2016-09-12 22:46:36]

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

https://codereview.chromium.org/2328973003/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.html:62: <button
id="delete-all">Delete all</button>
On 2016/09/12 20:26:01, chili wrote:
> nit: can we id this to delete-all-requests and delete-selected-requests?  
This
> will help with any name confusion with clear stored pages vs delete here (I'll
> make a note to rename the cleared pages ids too)

Done.

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

https://codereview.chromium.org/2328973003/diff/40001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.js:154:
$('page-actions-info').textContent = status;
On 2016/09/12 20:26:01, chili wrote:
> page-actions-info is a div between the stored pages table and request queue
> table.  Should we have another info? (i.e. have stored-pages-actions-info and
> request-queue-actions-info)?

Done.

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

https://codereview.chromium.org/2328973003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:60:
return "Store failure";
On 2016/09/12 20:26:02, chili wrote:
> should we have slightly better messaging for which request failed to delete?

We don't have the URL, but we do have the request id.  Added request IDs to the
return string.

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

https://codereview.chromium.org/2328973003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:64: void
DeleteAllGetCallback(
On 2016/09/12 20:26:02, chili wrote:
> nit: this method name is really confusing...  what about...
> "HandleGetAllRequestsForDelete"? or "OnGetAllRequestsForDelete"?

How about HandleGetAllRequestsForDeleteCallback.  (I left the word callback in
the function title to help us realized that it is part of an async processing
chain.)

[chili, 2016-09-12 23:09:48]

Sorry about the back-and-forth on the message

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

https://codereview.chromium.org/2328973003/diff/80001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.html:80: <div
id="request-queue-actions-info"></div>
ui-nit: I think this should be moved after line 75 (after the </table>)

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

https://codereview.chromium.org/2328973003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:64:
return "Store failure, could not delete urls: " + failed_ids;
nit: "Store failure, could not delete ids: " ?

I'm happy with IDs.  Something to think about though - we don't actually show
request_ids anywhere in the table UI.  Would a list of failed IDs be useful
somehow?  If not, perhaps just having a message like "Deletion failed for one or
more requests" would suffice

[chili, 2016-09-12 23:10:36]

and lgtm

[Pete Williamson, 2016-09-12 23:11:55]

The CQ bit was checked by petewil@chromium.org

[commit-bot: I haz the power, 2016-09-12 23:13:01]

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-09-12 23:20:22]

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

[commit-bot: I haz the power, 2016-09-12 23:20:23]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Pete Williamson, 2016-09-13 00:47:56]

petewil@chromium.org changed reviewers:
+ bauerb@chromium.org

[Pete Williamson, 2016-09-13 00:47:56]

BauerB: Please review as OWNER.

This change is adding two new buttons to the chrome:offline-internals page to
allow deleting SavePageRequest items from the background offlining request
queue.

Thanks!

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

https://codereview.chromium.org/2328973003/diff/80001/chrome/browser/resource...
chrome/browser/resources/offline_pages/offline_internals.html:80: <div
id="request-queue-actions-info"></div>
On 2016/09/12 23:09:48, chili wrote:
> ui-nit: I think this should be moved after line 75 (after the </table>)  

Done.

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

https://codereview.chromium.org/2328973003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:64:
return "Store failure, could not delete urls: " + failed_ids;
On 2016/09/12 23:09:48, chili wrote:
> nit: "Store failure, could not delete ids: " ?
> 
> I'm happy with IDs.  Something to think about though - we don't actually show
> request_ids anywhere in the table UI.  Would a list of failed IDs be useful
> somehow?  If not, perhaps just having a message like "Deletion failed for one
or
> more requests" would suffice

Good point, the request ID is not that helpful.  Maybe a less informational
error string is better.

[Bernhard Bauer, 2016-09-13 08:29:05]

lgtm

https://codereview.chromium.org/2328973003/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2328973003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:57:
failed = true;
You could return here right away.

https://codereview.chromium.org/2328973003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:125:
args->GetList(1, &offline_ids_from_arg);
If GetList() fails, you'll access an uninitialized pointer. Initialize it with
null or check the return value?

[Pete Williamson, 2016-09-13 17:03:27]

https://codereview.chromium.org/2328973003/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2328973003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:57:
failed = true;
On 2016/09/13 08:29:05, Bernhard Bauer wrote:
> You could return here right away.

Done.

https://codereview.chromium.org/2328973003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:125:
args->GetList(1, &offline_ids_from_arg);
On 2016/09/13 08:29:05, Bernhard Bauer wrote:
> If GetList() fails, you'll access an uninitialized pointer. Initialize it with
> null or check the return value?

Done.

[Pete Williamson, 2016-09-13 17:03:32]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-09-13 17:03:32]

The patchset sent to the CQ was uploaded after l-g-t-m from chili@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2328973003/#ps140001
(title: "CR feedback per BauerB")

[commit-bot: I haz the power, 2016-09-13 17:03:54]

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-09-13 18:09:01]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-13 18:10:22]

Description was changed from

==========
Adds a delete button to chrome:offline-internals

We currently don't have a way to delete 'stuck' requests to get back
to a clean state, or to remove requests that are in the way of testing.
This new button will allow us to do just that.

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

to

==========
Adds a delete button to chrome:offline-internals

We currently don't have a way to delete 'stuck' requests to get back
to a clean state, or to remove requests that are in the way of testing.
This new button will allow us to do just that.

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

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

[commit-bot: I haz the power, 2016-09-13 18:10:23]

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