[Offline Pages] Showing expired pages in internal page.

[Offline Pages] Showing expired pages in internal page.

Add a new column in offline internal page showing if a page is expired.

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

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

[romax, 2016-07-12 00:01:48]

Description was changed from

==========
[Offline Pages] Showing expired pages in internal page.

Add a new column in offline internal page showing if a page is expired.

BUG=627293
==========

to

==========
[Offline Pages] Showing expired pages in internal page.

Add a new column in offline internal page showing if a page is expired.

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

[romax, 2016-07-12 00:10:23]

romax@chromium.org changed reviewers:
+ dewittj@chromium.org, michaelpg@chromium.org

[romax, 2016-07-12 00:10:23]

dewittj@chromium.org: Please review changes in components/

michaelpg@chromium.org: Please review changes in chrome/browser

Thanks!

[dewittj, 2016-07-12 21:57:20]

lgtm other than one comment.

https://codereview.chromium.org/2142703002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2142703002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:453:
weak_ptr_factory_.GetWeakPtr(), false, callback));
I slightly prefer an enum class or a bitfield here to a boolean just for
readability.

[michaelpg, 2016-07-13 00:09:21]

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

https://codereview.chromium.org/2142703002/diff/1/chrome/browser/resources/of...
chrome/browser/resources/offline_pages/offline_internals.js:52: cell.textContent
= pages[i].expired;
This will implicitly set textContent to 'true' or 'false' if I understand this
CL correctly. Please use an explicit string:
    pages[i].expired.toString()
or 'Yes'/'No' or whatever.

(Is that really what you want? Or would something like a "Status" column with a
possible "Expired" value make more sense?)

[romax, 2016-07-13 20:01:08]

addressed comments, PTAL. Thanks!

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

https://codereview.chromium.org/2142703002/diff/1/chrome/browser/resources/of...
chrome/browser/resources/offline_pages/offline_internals.js:52: cell.textContent
= pages[i].expired;
On 2016/07/13 00:09:21, michaelpg (slow) wrote:
> This will implicitly set textContent to 'true' or 'false' if I understand this
> CL correctly. Please use an explicit string:
>     pages[i].expired.toString()
> or 'Yes'/'No' or whatever.
> 
> (Is that really what you want? Or would something like a "Status" column with
a
> possible "Expired" value make more sense?)

I'll change it to Yes/No and I think it's enough. A 'status' column would have
some empty cells for the not-expired pages.

https://codereview.chromium.org/2142703002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2142703002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:453:
weak_ptr_factory_.GetWeakPtr(), false, callback));
On 2016/07/12 21:57:20, dewittj wrote:
> I slightly prefer an enum class or a bitfield here to a boolean just for
> readability.

Done.

[dewittj, 2016-07-13 20:28:07]

lgtm

[michaelpg, 2016-07-14 00:05:13]

lgtm

[romax, 2016-07-14 00:19:19]

The CQ bit was checked by romax@chromium.org

[commit-bot: I haz the power, 2016-07-14 00:20:17]

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-07-14 00:42:07]

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

[commit-bot: I haz the power, 2016-07-14 00:42:08]

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

[romax, 2016-07-14 01:20:49]

The CQ bit was checked by romax@chromium.org

[romax, 2016-07-14 01:20:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org, dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2142703002/#ps40001
(title: "fix tests.")

[commit-bot: I haz the power, 2016-07-14 01:21:49]

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-07-14 02:40:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-14 02:40:40]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 02:42:45]

Description was changed from

==========
[Offline Pages] Showing expired pages in internal page.

Add a new column in offline internal page showing if a page is expired.

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

to

==========
[Offline Pages] Showing expired pages in internal page.

Add a new column in offline internal page showing if a page is expired.

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

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

[commit-bot: I haz the power, 2016-07-14 02:42:47]

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