Track if a ReadingListEntry has been dismissed

Track if a ReadingListEntry has been dismissed

This CL adds a field to the ReadingListEntry, saved only on the device, to
track if the entry has been dismissed from the NTP.
The only way to change the extra information is to use the model, allowing
the data to be stored.

BUG=707730
Review-Url: https://codereview.chromium.org/2806213004
Cr-Commit-Position: refs/heads/master@{#463615}
Committed: https://chromium.googlesource.com/chromium/src/+/0043013229fffab3555daf28c64ee27521209d7b

[gambard, 2017-04-10 15:49:47]

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

[gambard, 2017-04-10 15:49:48]

PTAL. I am not sure about the names or the declaration in the reading_list
namespace.

[Olivier, 2017-04-10 15:58:23]

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
File components/reading_list/core/reading_list_entry.cc (right):

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
components/reading_list/core/reading_list_entry.cc:412: if
(pb_extra.has_dismissed()) {
This is true because the only field if dismissed. But you may want to create the
ContentSuggestionsExtra outside of this if.

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
File components/reading_list/core/reading_list_entry.h (right):

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
components/reading_list/core/reading_list_entry.h:112:
reading_list::ContentSuggestionsExtra* content_suggestions_extra() const;
Are you using this method to set the dismissed field? Or will there be a setter?

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
components/reading_list/core/reading_list_entry.h:224:
std::unique_ptr<reading_list::ContentSuggestionsExtra>
nit: why not just a reading_list::ContentSuggestionsExtra?
This will init it by default.

[gambard, 2017-04-11 08:11:45]

Thanks, PTAL.

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
File components/reading_list/core/reading_list_entry.cc (right):

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
components/reading_list/core/reading_list_entry.cc:412: if
(pb_extra.has_dismissed()) {
On 2017/04/10 15:58:23, Olivier Robin wrote:
> This is true because the only field if dismissed. But you may want to create
the
> ContentSuggestionsExtra outside of this if.

Done.

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
File components/reading_list/core/reading_list_entry.h (right):

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
components/reading_list/core/reading_list_entry.h:112:
reading_list::ContentSuggestionsExtra* content_suggestions_extra() const;
On 2017/04/10 15:58:23, Olivier Robin wrote:
> Are you using this method to set the dismissed field? Or will there be a
setter?

This method returns the struct, I set it directly in the struct. I don't want to
add to the Reading List entry one method for every field in the extras.

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
components/reading_list/core/reading_list_entry.h:224:
std::unique_ptr<reading_list::ContentSuggestionsExtra>
On 2017/04/10 15:58:23, Olivier Robin wrote:
> nit: why not just a reading_list::ContentSuggestionsExtra?
> This will init it by default.

I used the same pattern as in content_suggestion.h, but I guess they are using
it because the extra are optional in their case.
I used an object

[Olivier, 2017-04-11 08:17:19]

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
File components/reading_list/core/reading_list_entry.h (right):

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
components/reading_list/core/reading_list_entry.h:112:
reading_list::ContentSuggestionsExtra* content_suggestions_extra() const;
On 2017/04/11 08:11:45, gambard wrote:
> On 2017/04/10 15:58:23, Olivier Robin wrote:
> > Are you using this method to set the dismissed field? Or will there be a
> setter?
> 
> This method returns the struct, I set it directly in the struct. I don't want
to
> add to the Reading List entry one method for every field in the extras.

This will likely cause problem. Most obvious one is that your change will not
persist in the datastore.

[gambard, 2017-04-11 09:42:56]

Thanks, PTAL.

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
File components/reading_list/core/reading_list_entry.h (right):

https://codereview.chromium.org/2806213004/diff/20001/components/reading_list...
components/reading_list/core/reading_list_entry.h:112:
reading_list::ContentSuggestionsExtra* content_suggestions_extra() const;
On 2017/04/11 08:17:19, Olivier Robin wrote:
> On 2017/04/11 08:11:45, gambard wrote:
> > On 2017/04/10 15:58:23, Olivier Robin wrote:
> > > Are you using this method to set the dismissed field? Or will there be a
> > setter?
> > 
> > This method returns the struct, I set it directly in the struct. I don't
want
> to
> > add to the Reading List entry one method for every field in the extras.
> 
> This will likely cause problem. Most obvious one is that your change will not
> persist in the datastore.

Done.

[Olivier, 2017-04-11 12:06:19]

https://codereview.chromium.org/2806213004/diff/60001/components/reading_list...
File components/reading_list/core/reading_list_entry.cc (right):

https://codereview.chromium.org/2806213004/diff/60001/components/reading_list...
components/reading_list/core/reading_list_entry.cc:231:
content_suggestions_extra_ = extra;
This will still not persist until you call some model method.

[gambard, 2017-04-11 12:14:37]

Thanks, PTAL.

https://codereview.chromium.org/2806213004/diff/60001/components/reading_list...
File components/reading_list/core/reading_list_entry.cc (right):

https://codereview.chromium.org/2806213004/diff/60001/components/reading_list...
components/reading_list/core/reading_list_entry.cc:231:
content_suggestions_extra_ = extra;
On 2017/04/11 12:06:18, Olivier Robin wrote:
> This will still not persist until you call some model method.

I was planning to do it in a future CL,  they are now merged.

[Olivier, 2017-04-11 12:36:59]

https://codereview.chromium.org/2806213004/diff/80001/components/reading_list...
File components/reading_list/core/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2806213004/diff/80001/components/reading_list...
components/reading_list/core/reading_list_model_impl.cc:469: auto iterator =
entries_->find(url);
GetMutableEntryFromURL

[gambard, 2017-04-11 12:52:56]

Thanks, PTAL.

https://codereview.chromium.org/2806213004/diff/80001/components/reading_list...
File components/reading_list/core/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2806213004/diff/80001/components/reading_list...
components/reading_list/core/reading_list_model_impl.cc:469: auto iterator =
entries_->find(url);
On 2017/04/11 12:36:59, Olivier Robin wrote:
> GetMutableEntryFromURL

Done.

[Olivier, 2017-04-11 12:57:49]

LGTM one comment

https://codereview.chromium.org/2806213004/diff/100001/components/reading_lis...
File components/reading_list/core/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2806213004/diff/100001/components/reading_lis...
components/reading_list/core/reading_list_model_impl.cc:469: ReadingListEntry*
entry = GetMutableEntryFromURL(url);
if ! nullptr

[gambard, 2017-04-11 13:22:20]

Description was changed from

==========
Track if a ReadingListEntry has been dismissed

This CL adds a field to the ReadingListEntry, saved only on the device, to
track if the entry has been dismissed from the NTP.

BUG=707730
==========

to

==========
Track if a ReadingListEntry has been dismissed

This CL adds a field to the ReadingListEntry, saved only on the device, to
track if the entry has been dismissed from the NTP.
The only way to change the extra information is to use the model, allowing
the data to be stored.

BUG=707730
==========

[gambard, 2017-04-11 13:22:28]

Thanks!

https://codereview.chromium.org/2806213004/diff/100001/components/reading_lis...
File components/reading_list/core/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2806213004/diff/100001/components/reading_lis...
components/reading_list/core/reading_list_model_impl.cc:469: ReadingListEntry*
entry = GetMutableEntryFromURL(url);
On 2017/04/11 12:57:49, Olivier Robin wrote:
> if ! nullptr

Done.

[gambard, 2017-04-11 13:22:35]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-04-11 13:22:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
olivierrobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2806213004/#ps120001
(title: "Address comment")

[commit-bot: I haz the power, 2017-04-11 13:22:52]

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

[Marc Treib, 2017-04-11 13:37:18]

treib@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2017-04-11 13:37:18]

Drive-by :)

https://codereview.chromium.org/2806213004/diff/120001/components/reading_lis...
File components/reading_list/core/reading_list_entry.h (right):

https://codereview.chromium.org/2806213004/diff/120001/components/reading_lis...
components/reading_list/core/reading_list_entry.h:112: const
reading_list::ContentSuggestionsExtra* ContentSuggestionsExtra() const;
Can this ever be null? If not, should it return a const ref instead?

Also, nit: "reading_list::" not required.

[commit-bot: I haz the power, 2017-04-11 14:05:42]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491916955409600,
"parent_rev": "48d0893d120389b02626a617e69fbf341f7f6909", "commit_rev":
"0043013229fffab3555daf28c64ee27521209d7b"}

[commit-bot: I haz the power, 2017-04-11 14:06:30]

Description was changed from

==========
Track if a ReadingListEntry has been dismissed

This CL adds a field to the ReadingListEntry, saved only on the device, to
track if the entry has been dismissed from the NTP.
The only way to change the extra information is to use the model, allowing
the data to be stored.

BUG=707730
==========

to

==========
Track if a ReadingListEntry has been dismissed

This CL adds a field to the ReadingListEntry, saved only on the device, to
track if the entry has been dismissed from the NTP.
The only way to change the extra information is to use the model, allowing
the data to be stored.

BUG=707730

Review-Url: https://codereview.chromium.org/2806213004
Cr-Commit-Position: refs/heads/master@{#463615}
Committed:
https://chromium.googlesource.com/chromium/src/+/0043013229fffab3555daf28c64e...
==========

[commit-bot: I haz the power, 2017-04-11 14:06:32]

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

[findit-for-me, 2017-04-11 14:53:36]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2811083004/ by
findit-for-me@appspot.gserviceaccount.com.

The reason for reverting is: 
Findit identified CL at revision 463615 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....

[stgao, 2017-04-11 17:38:27]

On 2017/04/11 14:53:36, findit-for-me wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/2811083004/ by
> mailto:findit-for-me@appspot.gserviceaccount.com.
> 
> The reason for reverting is: 
> Findit identified CL at revision 463615 as the culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....

This is a false positive due to flaky compile. Incorrect config for dependency
on a generated header file net/net_features.h