Add Distillation info to Reading List view.

Add Distillation info to Reading List view.

If page is distilled, show
- Date of distillation in relative format (1 hour ago, 2 days ago).
- The size of distillation files in human readable format.

BUG=694451
Review-Url: https://codereview.chromium.org/2751833005
Cr-Commit-Position: refs/heads/master@{#460426}
Committed: https://chromium.googlesource.com/chromium/src/+/0ac7ceb17518903b0072836ab722384bad87e24e

[Olivier, 2017-03-15 18:04:11]

olivierrobin@chromium.org changed reviewers:
+ gambard@chromium.org

[Olivier, 2017-03-15 18:04:11]

Hi
PTAL
ui/ part in review at https://codereview.chromium.org/2747253004/

[gambard, 2017-03-16 07:50:24]

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
(right):

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:606:
if (oldItem.url == newItem.url && oldItem.type == newItem.type) {
This is my main concern.
I don't see how this would be true anymore?
I think you are reloading all the collection every time one entry is distilled.

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h
(right):

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h:19: //
The main text to display.
Update comment

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h:21: //
The secondary text to display.
Update comment

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h:62: //
Detail label.
Update comment

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm
(right):

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:30: //
Margin for the elements displayed in the cell.
Update the comment

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:32: //
Margin for the elements displayed in the cell.
Change this comment (this is the margin between the details and the rest of the
content right?)

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:385: //
Place the download indicator in the bottom right corner of the favicon.
This comment should be below the added constraints.

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:386:
[_distillationDateLabel.topAnchor
Add a comment for the constraints

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:443: //
Ellapsed is negative as required by TimeFormat.
Are you sure it is negative? |now| should be > |distillationDate| no? And you
are testing for |elapsed| to be lower than something to see if it is in the
future.

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:443: //
Ellapsed is negative as required by TimeFormat.
s/Ellapsed/Elapsed

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:466:
self.subtitleLabel.text = nil;
You should probably set the date and size to zero also.

https://codereview.chromium.org/2751833005/diff/20001/ui/base/l10n/formatter.cc
File ui/base/l10n/formatter.cc (right):

https://codereview.chromium.org/2751833005/diff/20001/ui/base/l10n/formatter....
ui/base/l10n/formatter.cc:34: static const Pluralities IDS_ELAPSED_LONG_SEC = {
Are those files supposed to be in this CL?

[Olivier, 2017-03-16 09:20:15]

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
(right):

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:606:
if (oldItem.url == newItem.url && oldItem.type == newItem.type) {
On 2017/03/16 07:50:23, gambard wrote:
> This is my main concern.
> I don't see how this would be true anymore?
> I think you are reloading all the collection every time one entry is
distilled.

I guess you don't reload the "Read" section.
Do you see a better way to do it?

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h
(right):

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h:19: //
The main text to display.
On 2017/03/16 07:50:23, gambard wrote:
> Update comment

Done.

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h:21: //
The secondary text to display.
On 2017/03/16 07:50:23, gambard wrote:
> Update comment

Done.

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h:62: //
Detail label.
On 2017/03/16 07:50:23, gambard wrote:
> Update comment

Done.

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm
(right):

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:30: //
Margin for the elements displayed in the cell.
On 2017/03/16 07:50:23, gambard wrote:
> Update the comment

Done.

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:32: //
Margin for the elements displayed in the cell.
On 2017/03/16 07:50:24, gambard wrote:
> Change this comment (this is the margin between the details and the rest of
the
> content right?)

Done.

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:385: //
Place the download indicator in the bottom right corner of the favicon.
On 2017/03/16 07:50:24, gambard wrote:
> This comment should be below the added constraints.

Done.

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:386:
[_distillationDateLabel.topAnchor
On 2017/03/16 07:50:23, gambard wrote:
> Add a comment for the constraints

Done.

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:443: //
Ellapsed is negative as required by TimeFormat.
On 2017/03/16 07:50:23, gambard wrote:
> Are you sure it is negative? |now| should be > |distillationDate| no? And you
> are testing for |elapsed| to be lower than something to see if it is in the
> future.

No, you are right, it is positive.

https://codereview.chromium.org/2751833005/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:466:
self.subtitleLabel.text = nil;
On 2017/03/16 07:50:24, gambard wrote:
> You should probably set the date and size to zero also.

Done.

https://codereview.chromium.org/2751833005/diff/20001/ui/base/l10n/formatter.cc
File ui/base/l10n/formatter.cc (right):

https://codereview.chromium.org/2751833005/diff/20001/ui/base/l10n/formatter....
ui/base/l10n/formatter.cc:34: static const Pluralities IDS_ELAPSED_LONG_SEC = {
On 2017/03/16 07:50:24, gambard wrote:
> Are those files supposed to be in this CL?

As said in message, this part is in another CL
(https://codereview.chromium.org/2747253004/).
I will remove it from this one.

[Olivier, 2017-03-17 15:14:14]

I changed the layout according to the bug.
Now all cells have same size.
PTAL.

[gambard, 2017-03-17 15:39:43]

I removed the stack views in the stack views because of crbug.com/698379.
Also the position of the text inside the cell is moved instantaneously, maybe
add an animation.

https://codereview.chromium.org/2751833005/diff/100001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
(right):

https://codereview.chromium.org/2751833005/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:603:
if (oldItem.url == newItem.url && oldItem.type == newItem.type) {
Do you need this test? oldItem.type == newItem.type

[Olivier, 2017-03-24 10:07:15]

Patchset #7 (id:120001) has been deleted

[Olivier, 2017-03-24 10:07:28]

Patchset #7 (id:140001) has been deleted

[Olivier, 2017-03-24 10:07:41]

Thanks.
PTAL

[gambard, 2017-03-24 16:21:27]

I need to think about the model used to display the item and its cell. Can this
wait for a few days?

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h
(right):

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h:59:
@property(nonatomic, readonly, strong) UILabel* distillationDateLabel;
Switch those ones with the one in the class extension (you are not using those
one outside of the cell).

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm
(right):

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:343:
_distillationDateLabel.textColor = [UIColor colorWithWhite:0 alpha:0.38];
Use a constant for 0.38.
And are you sure you want alpha 0.38 and not alpha 1 and white 0.38?

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:385:
@"H:|-0-[date]-(>=margin)-[size]-0-|",
Can you try this constraint with a list of items containing distilled and
non-distilled items with enough cells to trigger reuse?

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:386:
@"V:|-0-[title]-0-[subtitle]",
I prefer |[title][subtitle] to |-0-[title]-0-[subtitle]

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:387:
@"H:|-(margin)-[favicon]-(margin)-[title]-(>=margin)-|",
favicon and title are not in the same view. I don't see how this constraint can
work?

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:388:
@"H:[favicon]-(margin)-[subtitle]-(>=margin)-|",
Same. I don't understand this constraint.
Don't you just want
H:|[subtitle]-(>=0)-|
H:|[title]-(>=0)-|

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:395:
//@"infoView": _infoView,
Why is it commented?

[Olivier, 2017-03-29 12:57:07]

Thanks

I rebased and addressed comments. PTAL.

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h
(right):

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h:59:
@property(nonatomic, readonly, strong) UILabel* distillationDateLabel;
On 2017/03/24 16:21:26, gambard wrote:
> Switch those ones with the one in the class extension (you are not using those
> one outside of the cell).

Done.

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm
(right):

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:343:
_distillationDateLabel.textColor = [UIColor colorWithWhite:0 alpha:0.38];
On 2017/03/24 16:21:26, gambard wrote:
> Use a constant for 0.38.
> And are you sure you want alpha 0.38 and not alpha 1 and white 0.38?

Mock say #000000 @38%.
I kind of interpreted this, but I may be wrong...

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:385:
@"H:|-0-[date]-(>=margin)-[size]-0-|",
On 2017/03/24 16:21:26, gambard wrote:
> Can you try this constraint with a list of items containing distilled and
> non-distilled items with enough cells to trigger reuse?

Done.

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:386:
@"V:|-0-[title]-0-[subtitle]",
On 2017/03/24 16:21:26, gambard wrote:
> I prefer |[title][subtitle] to |-0-[title]-0-[subtitle]

Done.

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:387:
@"H:|-(margin)-[favicon]-(margin)-[title]-(>=margin)-|",
On 2017/03/24 16:21:26, gambard wrote:
> favicon and title are not in the same view. I don't see how this constraint
can
> work?

They are both in the same super view and _textView is fitting to the content.

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:388:
@"H:[favicon]-(margin)-[subtitle]-(>=margin)-|",
On 2017/03/24 16:21:26, gambard wrote:
> Same. I don't understand this constraint.
> Don't you just want
> H:|[subtitle]-(>=0)-|
> H:|[title]-(>=0)-|

Constraints are not necessarily on the superview, so I think this works.
I can add constraints on _textView but this will likely increase the number of
constraints.

_textView is only a facilitator that is used to center the text.
I changed to use a UILayoutGuide instead.

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:395:
//@"infoView": _infoView,
On 2017/03/24 16:21:26, gambard wrote:
> Why is it commented?

Done.

[gambard, 2017-03-29 13:30:38]

lgtm with 1 comment

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm
(right):

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:343:
_distillationDateLabel.textColor = [UIColor colorWithWhite:0 alpha:0.38];
On 2017/03/29 12:57:06, Olivier Robin wrote:
> On 2017/03/24 16:21:26, gambard wrote:
> > Use a constant for 0.38.
> > And are you sure you want alpha 0.38 and not alpha 1 and white 0.38?
> 
> Mock say #000000 @38%.
> I kind of interpreted this, but I may be wrong...

I don't know :)

https://codereview.chromium.org/2751833005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:388:
@"H:[favicon]-(margin)-[subtitle]-(>=margin)-|",
On 2017/03/29 12:57:06, Olivier Robin wrote:
> On 2017/03/24 16:21:26, gambard wrote:
> > Same. I don't understand this constraint.
> > Don't you just want
> > H:|[subtitle]-(>=0)-|
> > H:|[title]-(>=0)-|
> 
> Constraints are not necessarily on the superview, so I think this works.
> I can add constraints on _textView but this will likely increase the number of
> constraints.
> 
> _textView is only a facilitator that is used to center the text.
> I changed to use a UILayoutGuide instead.

Acknowledged.

https://codereview.chromium.org/2751833005/diff/180001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
(right):

https://codereview.chromium.org/2751833005/diff/180001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:1179:
header = [[CollectionViewTextItem alloc] initWithType:ItemTypeHeader];
Move this out of the switch?

[Olivier, 2017-03-29 14:15:15]

Thanks

https://codereview.chromium.org/2751833005/diff/180001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
(right):

https://codereview.chromium.org/2751833005/diff/180001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:1179:
header = [[CollectionViewTextItem alloc] initWithType:ItemTypeHeader];
On 2017/03/29 13:30:38, gambard wrote:
> Move this out of the switch?

Done.

[Olivier, 2017-03-29 14:15:19]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-03-29 14:15:20]

The patchset sent to the CQ was uploaded after l-g-t-m from gambard@chromium.org
Link to the patchset: https://codereview.chromium.org/2751833005/#ps200001
(title: "feedback")

[commit-bot: I haz the power, 2017-03-29 14:15:46]

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-03-29 14:25:22]

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

[commit-bot: I haz the power, 2017-03-29 14:25:25]

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

[Olivier, 2017-03-29 16:03:22]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-03-29 16:03:22]

The patchset sent to the CQ was uploaded after l-g-t-m from gambard@chromium.org
Link to the patchset: https://codereview.chromium.org/2751833005/#ps220001
(title: "fix test")

[commit-bot: I haz the power, 2017-03-29 16:04:35]

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-03-29 16:54:24]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490803402212780,
"parent_rev": "f2cdf4645352446d423e8b7c286870bcb7d00797", "commit_rev":
"0ac7ceb17518903b0072836ab722384bad87e24e"}

[commit-bot: I haz the power, 2017-03-29 16:55:09]

Description was changed from

==========
Add Distillation info to Reading List view.

If page is distilled, show
- Date of distillation in relative format (1 hour ago, 2 days ago).
- The size of distillation files in human readable format.

BUG=694451
==========

to

==========
Add Distillation info to Reading List view.

If page is distilled, show
- Date of distillation in relative format (1 hour ago, 2 days ago).
- The size of distillation files in human readable format.

BUG=694451

Review-Url: https://codereview.chromium.org/2751833005
Cr-Commit-Position: refs/heads/master@{#460426}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ac7ceb17518903b0072836ab722...
==========

[commit-bot: I haz the power, 2017-03-29 16:55:13]

Committed patchset #10 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/0ac7ceb17518903b0072836ab722...