MD History: Refresh the list when clearing browsing data

MD History: Refresh the list when clearing browsing data

Automatically refresh history list when user clears browsing data from
chrome://settings.

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

Committed: https://crrev.com/570a8850893d9625d9aeb2b5fa7d27aa35f17d3b
Cr-Commit-Position: refs/heads/master@{#413020}

[lshang, 2016-08-03 06:40:42]

Description was changed from

==========
MD History: Refresh the list when clearing browsing data


Merge branch 'master' into MDH_clear_history


add one more log


Merge branch 'master' into MDH_clear_history


draft

BUG=
==========

to

==========
MD History: Refresh the list when clearing browsing data


Merge branch 'master' into MDH_clear_history


add one more log


Merge branch 'master' into MDH_clear_history


draft

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

[lshang, 2016-08-03 06:49:27]

Description was changed from

==========
MD History: Refresh the list when clearing browsing data


Merge branch 'master' into MDH_clear_history


add one more log


Merge branch 'master' into MDH_clear_history


draft

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

to

==========
MD History: Refresh the list when clearing browsing data

Automatically refresh history list when user clears browsing data from 
chrome://settings.

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

[lshang, 2016-08-04 00:17:03]

lshang@chromium.org changed reviewers:
+ calamity@chromium.org, tsergeant@chromium.org

[lshang, 2016-08-04 00:17:05]

PTAL thanks!

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (left):

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:87: this.push.apply(this,
results);
These two lines seem not right.

When there are new items coming and the list is refreshed automatically, the
fetched 'results' will include all updated history items including new items and
old items, and 'unshift' method will append 'results' to historyData_, which
will result in historyData_ having 2 copies of old items?

It hasn't caused any problems, which I think is because we now don't support
refreshing history list automatically when user navigates to new websites, so
that these code only get executed when search term changes, which will clear
historyData_ ahead on line 78.

Please correct me if I understand it wrong:-)

[Dan Beam, 2016-08-04 00:30:46]

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:90: /** @type
{HistoryListContainerElement} */($('history-app').$['history'])
we really need a lint rule about thing.$.localId

everybody i've talked to has agreed to treat this basically like private state
but i keep seeing this

[tsergeant, 2016-08-04 01:25:55]

Can you please check that this works with grouped history? I think it should be
fine, but best to check.

At the moment, you can enable it by pasting

$('history-app').grouped_ = true;

in the devtools console.

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:90: /** @type
{HistoryListContainerElement} */($('history-app').$['history'])
On 2016/08/04 00:30:46, Dan Beam wrote:
> we really need a lint rule about thing.$.localId
> 
> everybody i've talked to has agreed to treat this basically like private state
> but i keep seeing this

Yup, please avoid directly accessing local DOM of other elements. You can either
add a getter for the <history-list-container> to <history-app>, or add a new
method to <history-app> to handle this callback (which is basically what we do
elsewhere in this file, even though it's not ideal).

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (left):

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:87: this.push.apply(this,
results);
On 2016/08/04 00:17:04, Liu Shang wrote:
> These two lines seem not right.
> 
> When there are new items coming and the list is refreshed automatically, the
> fetched 'results' will include all updated history items including new items
and
> old items, and 'unshift' method will append 'results' to historyData_, which
> will result in historyData_ having 2 copies of old items?
> 
> It hasn't caused any problems, which I think is because we now don't support
> refreshing history list automatically when user navigates to new websites, so
> that these code only get executed when search term changes, which will clear
> historyData_ ahead on line 78.
> 
> Please correct me if I understand it wrong:-)

These lines support infinite scrolling. When we do an incremental query, we
receive new data which gets appended directly after the old data.

You'll need to keep this logic in place while adding in separate logic for
refreshing the list.

[Dan Beam, 2016-08-04 03:23:43]

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

[Dan Beam, 2016-08-04 03:23:44]

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:90: /** @type
{HistoryListContainerElement} */($('history-app').$['history'])
On 2016/08/04 01:25:55, tsergeant wrote:
> On 2016/08/04 00:30:46, Dan Beam wrote:
> > we really need a lint rule about thing.$.localId
> > 
> > everybody i've talked to has agreed to treat this basically like private
state
> > but i keep seeing this
> 
> Yup, please avoid directly accessing local DOM of other elements. You can
either
> add a getter for the <history-list-container> to <history-app>, or add a new
> method to <history-app> to handle this callback (which is basically what we do
> elsewhere in this file, even though it's not ideal).

https://codereview.chromium.org/2209053002/

[lshang, 2016-08-04 08:40:24]

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

[commit-bot: I haz the power, 2016-08-04 08:41:25]

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, 2016-08-04 08:54:46]

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

[commit-bot: I haz the power, 2016-08-04 08:54:46]

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

[lshang, 2016-08-05 03:20:16]

Patchset #3 (id:40001) has been deleted

[lshang, 2016-08-05 03:34:36]

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

[commit-bot: I haz the power, 2016-08-05 03:34:54]

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

[lshang, 2016-08-05 03:39:03]

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:90: /** @type
{HistoryListContainerElement} */($('history-app').$['history'])
On 2016/08/04 01:25:55, tsergeant wrote:
> On 2016/08/04 00:30:46, Dan Beam wrote:
> > we really need a lint rule about thing.$.localId
> > 
> > everybody i've talked to has agreed to treat this basically like private
state
> > but i keep seeing this
> 
> Yup, please avoid directly accessing local DOM of other elements. You can
either
> add a getter for the <history-list-container> to <history-app>, or add a new
> method to <history-app> to handle this callback (which is basically what we do
> elsewhere in this file, even though it's not ideal).

Done.

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (left):

https://codereview.chromium.org/2200233003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:87: this.push.apply(this,
results);
On 2016/08/04 01:25:55, tsergeant wrote:
> On 2016/08/04 00:17:04, Liu Shang wrote:
> > These two lines seem not right.
> > 
> > When there are new items coming and the list is refreshed automatically, the
> > fetched 'results' will include all updated history items including new items
> and
> > old items, and 'unshift' method will append 'results' to historyData_, which
> > will result in historyData_ having 2 copies of old items?
> > 
> > It hasn't caused any problems, which I think is because we now don't support
> > refreshing history list automatically when user navigates to new websites,
so
> > that these code only get executed when search term changes, which will clear
> > historyData_ ahead on line 78.
> > 
> > Please correct me if I understand it wrong:-)
> 
> These lines support infinite scrolling. When we do an incremental query, we
> receive new data which gets appended directly after the old data.
> 
> You'll need to keep this logic in place while adding in separate logic for
> refreshing the list.

Ah, it's for infinite scrolling! Then it makes sense now. I've added them back.

[commit-bot: I haz the power, 2016-08-05 05:01:01]

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

[commit-bot: I haz the power, 2016-08-05 05:01:02]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-08-05 17:58:06]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[tsergeant, 2016-08-08 05:05:12]

calamity@, since you wrote most of the querying code, what do you think?

https://codereview.chromium.org/2200233003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/2200233003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:80: if
(this.lastSearchedTerm_ != this.searchedTerm) {
I'm a concerned about making the logic below more complicated, especially since
this is partly the same as the logic here for when the search term changes.

In fact, things from here like 
  this.resultLoadingDisabled_ = false;
probably need to be run when the list reloads.

Can we instead just check here whether the query is incremental or not? If it is
not an incremental query (a new search, or list reload), we should clear the
list using this block before we do anything with the new results.

list_container.js already knows whether a query was incremental, so we should be
able to pass that down to use it here.

https://codereview.chromium.org/2200233003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:94: var selectedItem =
this.historyData_.filter(function(item) {
Chris has a CL in review which makes it much easier to determine whether items
are selected, and works for both normal history and grouped history.

It should make things nicer for this patch, do you mind waiting until Chris'
patch lands?

[calamity, 2016-08-08 05:55:07]

https://codereview.chromium.org/2200233003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/2200233003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:80: if
(this.lastSearchedTerm_ != this.searchedTerm) {
On 2016/08/08 05:05:12, tsergeant wrote:
> I'm a concerned about making the logic below more complicated, especially
since
> this is partly the same as the logic here for when the search term changes.
> 
> In fact, things from here like 
>   this.resultLoadingDisabled_ = false;
> probably need to be run when the list reloads.
> 
> Can we instead just check here whether the query is incremental or not? If it
is
> not an incremental query (a new search, or list reload), we should clear the
> list using this block before we do anything with the new results.
> 
> list_container.js already knows whether a query was incremental, so we should
be
> able to pass that down to use it here.

Good idea! This code was written before the incremental vs non-incremental
change and the benefit of plumbing it here was overlooked. This is a solution
that is more semantically correct. Wew.

[lshang, 2016-08-09 00:49:26]

On 2016/08/08 05:05:12, tsergeant wrote:
> calamity@, since you wrote most of the querying code, what do you think?
> 
>
https://codereview.chromium.org/2200233003/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/md_history/history_list.js (right):
> 
>
https://codereview.chromium.org/2200233003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/md_history/history_list.js:80: if
> (this.lastSearchedTerm_ != this.searchedTerm) {
> I'm a concerned about making the logic below more complicated, especially
since
> this is partly the same as the logic here for when the search term changes.
> 
> In fact, things from here like 
>   this.resultLoadingDisabled_ = false;
> probably need to be run when the list reloads.
> 
> Can we instead just check here whether the query is incremental or not? If it
is
> not an incremental query (a new search, or list reload), we should clear the
> list using this block before we do anything with the new results.
> 
> list_container.js already knows whether a query was incremental, so we should
be
> able to pass that down to use it here.
> 
>
https://codereview.chromium.org/2200233003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/md_history/history_list.js:94: var selectedItem =
> this.historyData_.filter(function(item) {
> Chris has a CL in review which makes it much easier to determine whether items
> are selected, and works for both normal history and grouped history.
> 
> It should make things nicer for this patch, do you mind waiting until Chris'
> patch lands?

Yep, let's wait for Chris' patch to land first:)

[calamity, 2016-08-12 04:23:40]

FYI, my patch has landed =D

[lshang, 2016-08-12 04:33:44]

On 2016/08/12 04:23:40, calamity wrote:
> FYI, my patch has landed =D

Yay~ Rebase is coming!

[lshang, 2016-08-16 08:34:11]

Patchset #4 (id:80001) has been deleted

[lshang, 2016-08-16 08:34:19]

Patchset #4 (id:100001) has been deleted

[lshang, 2016-08-16 08:40:00]

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

[commit-bot: I haz the power, 2016-08-16 08:40:23]

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, 2016-08-16 09:09:18]

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

[commit-bot: I haz the power, 2016-08-16 09:09:19]

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

[lshang, 2016-08-17 05:50:58]

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

[commit-bot: I haz the power, 2016-08-17 05:51:25]

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

[lshang, 2016-08-17 06:00:11]

This CL has been updated. PTAL thanks!

Also have checked with grouped list, works fine.

The change in history_list_test.js seems a little bit not-pretty, since I need
to explicitly set queryState.incremental to be 'true' to load multiply set of
history results, and set it to 'false' so that next test case will clear current
results and load in new ones.

[commit-bot: I haz the power, 2016-08-17 06:48:07]

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

[commit-bot: I haz the power, 2016-08-17 06:48:10]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2016-08-17 06:58:28]

I'm not sure what the best way to handle tests is. Maybe we can add a new test
helper function that does some of the logic we skip by not using queryHistory
(calamity?)

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:220:
this.$['history'].clearingHistory();
Nit: this.$.history

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:222:
this.$['history'].queryHistory(false);
Maybe move this call into the clearHistory() method.

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:17: lastSearchedTerm_:
String,
This is no longer used and can be removed.

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/list_container.js (right):

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/list_container.js:48:
this.getSelectedItemCount());
For simplicity, I would suggest only checking the selected item count before you
reload the list (in clearHistory below). If there are items selected, exit
without trying to reload.

That way, you don't need to store clearingHistory_ and it simplifies the logic a
lot. It's possible for the reload to go through when it shouldn't, but that case
would be very difficult to trigger.

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/list_container.js:91: clearingHistory:
function() {
For method names, use the plain verb form:

clearHistory()

For properties, you can still use the participle form:

this.clearingHistory_

to indicate that it's a continuous state.

[lshang, 2016-08-18 01:08:46]

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

[commit-bot: I haz the power, 2016-08-18 01:09:10]

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

[lshang, 2016-08-18 01:16:38]

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

[commit-bot: I haz the power, 2016-08-18 01:17:25]

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, 2016-08-18 02:19:36]

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

[commit-bot: I haz the power, 2016-08-18 02:19:37]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-08-18 08:16:39]

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

[commit-bot: I haz the power, 2016-08-18 08:16:54]

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

[lshang, 2016-08-18 08:41:54]

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:220:
this.$['history'].clearingHistory();
On 2016/08/17 06:58:28, tsergeant wrote:
> Nit: this.$.history

Done.

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:222:
this.$['history'].queryHistory(false);
On 2016/08/17 06:58:28, tsergeant wrote:
> Maybe move this call into the clearHistory() method.

Done.

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:17: lastSearchedTerm_:
String,
On 2016/08/17 06:58:28, tsergeant wrote:
> This is no longer used and can be removed.

Done. Good catch!

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/list_container.js (right):

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/list_container.js:48:
this.getSelectedItemCount());
On 2016/08/17 06:58:28, tsergeant wrote:
> For simplicity, I would suggest only checking the selected item count before
you
> reload the list (in clearHistory below). If there are items selected, exit
> without trying to reload.
> 
> That way, you don't need to store clearingHistory_ and it simplifies the logic
a
> lot. It's possible for the reload to go through when it shouldn't, but that
case
> would be very difficult to trigger. 

Done. Great idea!

https://codereview.chromium.org/2200233003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/list_container.js:91: clearingHistory:
function() {
On 2016/08/17 06:58:28, tsergeant wrote:
> For method names, use the plain verb form:
> 
> clearHistory()
> 
> For properties, you can still use the participle form:
> 
> this.clearingHistory_
> 
> to indicate that it's a continuous state.

Done.

[commit-bot: I haz the power, 2016-08-18 10:15:39]

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

[commit-bot: I haz the power, 2016-08-18 10:15:40]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2016-08-19 00:31:01]

lgtm, thanks.

[lshang, 2016-08-19 01:36:04]

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

[commit-bot: I haz the power, 2016-08-19 01:36:41]

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, 2016-08-19 01:40:27]

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

[commit-bot: I haz the power, 2016-08-19 01:40:28]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-08-19 01:41:16]

The CQ bit was checked by lshang@chromium.org

[commit-bot: I haz the power, 2016-08-19 01:41:48]

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-08-19 01:49:05]

Description was changed from

==========
MD History: Refresh the list when clearing browsing data

Automatically refresh history list when user clears browsing data from 
chrome://settings.

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

to

==========
MD History: Refresh the list when clearing browsing data

Automatically refresh history list when user clears browsing data from 
chrome://settings.

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

[commit-bot: I haz the power, 2016-08-19 01:49:07]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2016-08-19 01:52:10]

Description was changed from

==========
MD History: Refresh the list when clearing browsing data

Automatically refresh history list when user clears browsing data from 
chrome://settings.

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

to

==========
MD History: Refresh the list when clearing browsing data

Automatically refresh history list when user clears browsing data from
chrome://settings.

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

Committed: https://crrev.com/570a8850893d9625d9aeb2b5fa7d27aa35f17d3b
Cr-Commit-Position: refs/heads/master@{#413020}
==========

[commit-bot: I haz the power, 2016-08-19 01:52:12]

Patchset 10 (id:??) landed as
https://crrev.com/570a8850893d9625d9aeb2b5fa7d27aa35f17d3b
Cr-Commit-Position: refs/heads/master@{#413020}