Remove ScopedVector usage from components/history

Remove ScopedVector usage from components/history

This CL replaces ScopedVector with std::vector<std::unique_ptr>
in components/history.
ScopedVector is deprecated, see bug.

BUG=554289
Review-Url: https://codereview.chromium.org/2906953003
Cr-Commit-Position: refs/heads/master@{#478547}
Committed: https://chromium.googlesource.com/chromium/src/+/370a037ae7d0cd617c45942610c5866aff44a35c

[leonhsl(Using Gerrit), 2017-05-27 09:07:34]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-27 09:07: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, 2017-05-27 09:17:10]

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

[commit-bot: I haz the power, 2017-05-27 09:17:11]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[donna.wu, 2017-05-27 09:23:57]

donna.wu@intel.com changed reviewers:
+ rockot@google.com, sky@chromium.org

[donna.wu, 2017-05-27 09:23:57]

Hi, sky, rocket and leon, would you please take a look at it? As a beginner, I
appreciate your help.

This CL replaced the deprecated ScopedVector usage under components/history
folder and modified a relevant file under chrome/browser/extensions.

[donna.wu, 2017-05-27 09:35:17]

donna.wu@intel.com changed reviewers:
+ rockot@chromium.org
- rockot@google.com

[leonhsl(Using Gerrit), 2017-05-27 12:34:25]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-27 12:34:35]

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, 2017-05-27 12:52:11]

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

[commit-bot: I haz the power, 2017-05-27 12:52:12]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sky, 2017-05-30 16:12:04]

sky@chromium.org changed reviewers:
+ brettw@chromium.org

[sky, 2017-05-30 16:12:05]

+brettw to see if he can think of a reason not to do as I suggest here.

https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
File components/history/core/browser/history_types.h (right):

https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
components/history/core/browser/history_types.h:138: typedef
std::vector<std::unique_ptr<URLResult>> URLResultVector;
I would be inclined to make this a vector<URLResult>.

[brettw, 2017-06-02 23:17:52]

https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
File components/history/core/browser/history_types.h (right):

https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
components/history/core/browser/history_types.h:138: typedef
std::vector<std::unique_ptr<URLResult>> URLResultVector;
On 2017/05/30 16:12:04, sky wrote:
> I would be inclined to make this a vector<URLResult>.

Sounds good to me! This was originally this way because there were no move
semantics and copying this structure is expensive. Currently, there are move
constructors for URLResult, and we already do vectors of URLResults.

In fact, in HistoryBackend::QueryHistoryText I see it create a
vector<URLResult>, and then do some complex dance with AppendURLBySwapping to
get a heap-allocated copy of that in the URLResult.

We should be able to delete AppendURLBySwapping and add a
SetURLResults(std::vector<URLResult>&& results) which moves a vector. This will
be much more efficient than the current code (moving a vector is almost free)
and save a bunch of heap allocations. and indirections.

[donna.wu, 2017-06-05 08:43:18]

https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
File components/history/core/browser/history_types.h (right):

https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
components/history/core/browser/history_types.h:138: typedef
std::vector<std::unique_ptr<URLResult>> URLResultVector;
On 2017/06/02 23:17:52, brettw wrote:
> On 2017/05/30 16:12:04, sky wrote:
> > I would be inclined to make this a vector<URLResult>.
> 
> Sounds good to me! This was originally this way because there were no move
> semantics and copying this structure is expensive. Currently, there are move
> constructors for URLResult, and we already do vectors of URLResults.
> 
> In fact, in HistoryBackend::QueryHistoryText I see it create a
> vector<URLResult>, and then do some complex dance with AppendURLBySwapping to
> get a heap-allocated copy of that in the URLResult.
> 
> We should be able to delete AppendURLBySwapping and add a
> SetURLResults(std::vector<URLResult>&& results) which moves a vector. This
will
> be much more efficient than the current code (moving a vector is almost free)
> and save a bunch of heap allocations. and indirections.

@brettw, it seems the appending method is still needed, we can not sure if the
user use the same QueryResults object to receive the results of multiple
QueryHistory callings or not. I saw a use case at Line 96(TestPaging function)
in components/history/core/browser/history_querying_unittest.cc.
There are two calling to AppendURLBySwapping in history_backend.cpp file, and
the first calling also need an appending operation.
SetURLResults() means replacing results, we will lost the previous result if the
result object reused.

https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
components/history/core/browser/history_types.h:138: typedef
std::vector<std::unique_ptr<URLResult>> URLResultVector;
On 2017/05/30 16:12:04, sky wrote:
> I would be inclined to make this a vector<URLResult>.

@sky, @brettw, how about using vector<URLResult*>? In this way, we can make
minimal changes, because it was perceived as this compliant type before, for
URLResult, the SwapResult method is actually more efficient than move
constructor, it can be used while appending new result to the vector and we can
keep the actual objects in heap.

[donna.wu, 2017-06-05 09:36:00]

The CQ bit was checked by donna.wu@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 09:36:09]

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, 2017-06-05 09:36:14]

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

[commit-bot: I haz the power, 2017-06-05 09:36:15]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[donna.wu, 2017-06-05 09:37:10]

On 2017/06/05 08:43:18, donna.wu wrote:
>
https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
> File components/history/core/browser/history_types.h (right):
> 
>
https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
> components/history/core/browser/history_types.h:138: typedef
> std::vector<std::unique_ptr<URLResult>> URLResultVector;
> On 2017/06/02 23:17:52, brettw wrote:
> > On 2017/05/30 16:12:04, sky wrote:
> > > I would be inclined to make this a vector<URLResult>.
> > 
> > Sounds good to me! This was originally this way because there were no move
> > semantics and copying this structure is expensive. Currently, there are move
> > constructors for URLResult, and we already do vectors of URLResults.
> > 
> > In fact, in HistoryBackend::QueryHistoryText I see it create a
> > vector<URLResult>, and then do some complex dance with AppendURLBySwapping
to
> > get a heap-allocated copy of that in the URLResult.
> > 
> > We should be able to delete AppendURLBySwapping and add a
> > SetURLResults(std::vector<URLResult>&& results) which moves a vector. This
> will
> > be much more efficient than the current code (moving a vector is almost
free)
> > and save a bunch of heap allocations. and indirections.
> 
> @brettw, it seems the appending method is still needed, we can not sure if the
> user use the same QueryResults object to receive the results of multiple
> QueryHistory callings or not. I saw a use case at Line 96(TestPaging function)
> in components/history/core/browser/history_querying_unittest.cc.
> There are two calling to AppendURLBySwapping in history_backend.cpp file, and
> the first calling also need an appending operation.
> SetURLResults() means replacing results, we will lost the previous result if
the
> result object reused.
> 
>
https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
> components/history/core/browser/history_types.h:138: typedef
> std::vector<std::unique_ptr<URLResult>> URLResultVector;
> On 2017/05/30 16:12:04, sky wrote:
> > I would be inclined to make this a vector<URLResult>.
> 
> @sky, @brettw, how about using vector<URLResult*>? In this way, we can make
> minimal changes, because it was perceived as this compliant type before, for
> URLResult, the SwapResult method is actually more efficient than move
> constructor, it can be used while appending new result to the vector and we
can
> keep the actual objects in heap.

The path has been updated by using vector<URLResult>. Would you please help to
trigger the trybot?

[Ken Rockot(use gerrit already), 2017-06-05 16:48:51]

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

[commit-bot: I haz the power, 2017-06-05 16:49:05]

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, 2017-06-05 17:56:26]

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

[commit-bot: I haz the power, 2017-06-05 17:56:27]

Dry run: This issue passed the CQ dry run.

[brettw, 2017-06-05 22:39:27]

https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
File components/history/core/browser/history_types.h (right):

https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
components/history/core/browser/history_types.h:138: typedef
std::vector<std::unique_ptr<URLResult>> URLResultVector;
We don't want to do that, who will free the pointers then? The whole point of
the unique_ptr is to get the ownership semantics correct. There is also no
desire to keep things on the heap.

The test you mentioned that relies on the append behavior should just be
updated. I think that test should not have been written that way. The normal
history APIs all give a new QueryResults object to the callback, so there is no
way for real code to depend on appending-to-an-existing-object behavior.

[donna.wu, 2017-06-06 14:48:15]

https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
File components/history/core/browser/history_types.h (right):

https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
components/history/core/browser/history_types.h:138: typedef
std::vector<std::unique_ptr<URLResult>> URLResultVector;
On 2017/06/05 22:39:27, brettw wrote:
> We don't want to do that, who will free the pointers then? The whole point of
> the unique_ptr is to get the ownership semantics correct. There is also no
> desire to keep things on the heap.
> 
> The test you mentioned that relies on the append behavior should just be
> updated. I think that test should not have been written that way. The normal
> history APIs all give a new QueryResults object to the callback, so there is
no
> way for real code to depend on appending-to-an-existing-object behavior.


Got it! I'm working on the solution and have a question about some other members
in Class QueryResults. If SetURLResults() replaced the result_ vector , the URL
to index map "url_to_results_" will need to be cleared before re-constructing
the entries accordingly and the "reached_beginning_" flag will always been set
to true after that. Right?

How about first_time_searched_, does it need to be reset? You mentioned "bug
1203054" about this member, but I can't find it. Is this bug still active? This
member(first_time_searched_) and its setter, getter have only been defined and
never been used. Is it OK to delete it?

[brettw, 2017-06-06 16:00:42]

On 2017/06/06 14:48:15, donna.wu wrote:
>
https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
> File components/history/core/browser/history_types.h (right):
> 
>
https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
> components/history/core/browser/history_types.h:138: typedef
> std::vector<std::unique_ptr<URLResult>> URLResultVector;
> On 2017/06/05 22:39:27, brettw wrote:
> > We don't want to do that, who will free the pointers then? The whole point
of
> > the unique_ptr is to get the ownership semantics correct. There is also no
> > desire to keep things on the heap.
> > 
> > The test you mentioned that relies on the append behavior should just be
> > updated. I think that test should not have been written that way. The normal
> > history APIs all give a new QueryResults object to the callback, so there is
> no
> > way for real code to depend on appending-to-an-existing-object behavior.
> 
> 
> Got it! I'm working on the solution and have a question about some other
members
> in Class QueryResults. If SetURLResults() replaced the result_ vector , the
URL
> to index map "url_to_results_" will need to be cleared before re-constructing
> the entries accordingly and the "reached_beginning_" flag will always been set
> to true after that. Right?

I didn't notice that, but you're right, you will need to recreate the
url_to_results_ map when the set function is called.

> How about first_time_searched_, does it need to be reset? You mentioned "bug
> 1203054" about this member, but I can't find it. Is this bug still active?
This
> member(first_time_searched_) and its setter, getter have only been defined and
> never been used. Is it OK to delete it?

It sure looks like first_time_searched_ and associated stuff can be deleted.

[donna.wu, 2017-06-07 07:08:09]

On 2017/06/06 16:00:42, brettw wrote:
> On 2017/06/06 14:48:15, donna.wu wrote:
> >
>
https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
> > File components/history/core/browser/history_types.h (right):
> > 
> >
>
https://codereview.chromium.org/2906953003/diff/20001/components/history/core...
> > components/history/core/browser/history_types.h:138: typedef
> > std::vector<std::unique_ptr<URLResult>> URLResultVector;
> > On 2017/06/05 22:39:27, brettw wrote:
> > > We don't want to do that, who will free the pointers then? The whole point
> of
> > > the unique_ptr is to get the ownership semantics correct. There is also no
> > > desire to keep things on the heap.
> > > 
> > > The test you mentioned that relies on the append behavior should just be
> > > updated. I think that test should not have been written that way. The
normal
> > > history APIs all give a new QueryResults object to the callback, so there
is
> > no
> > > way for real code to depend on appending-to-an-existing-object behavior.
> > 
> > 
> > Got it! I'm working on the solution and have a question about some other
> members
> > in Class QueryResults. If SetURLResults() replaced the result_ vector , the
> URL
> > to index map "url_to_results_" will need to be cleared before
re-constructing
> > the entries accordingly and the "reached_beginning_" flag will always been
set
> > to true after that. Right?
> 
> I didn't notice that, but you're right, you will need to recreate the
> url_to_results_ map when the set function is called.
> 
> > How about first_time_searched_, does it need to be reset? You mentioned "bug
> > 1203054" about this member, but I can't find it. Is this bug still active?
> This
> > member(first_time_searched_) and its setter, getter have only been defined
and
> > never been used. Is it OK to delete it?
> 
> It sure looks like first_time_searched_ and associated stuff can be deleted.

@brettw @sky the CL has been updated according to the comments.

[brettw, 2017-06-07 21:06:03]

lgtm

[donna.wu, 2017-06-08 03:10:10]

On 2017/06/07 21:06:03, brettw wrote:
> lgtm

Thank you!

[donna.wu, 2017-06-08 03:11:17]

The CQ bit was checked by donna.wu@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 03:11:40]

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, 2017-06-08 04:16:53]

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

[commit-bot: I haz the power, 2017-06-08 04:16:54]

Dry run: This issue passed the CQ dry run.

[donna.wu, 2017-06-08 07:03:19]

The CQ bit was unchecked by donna.wu@intel.com

[donna.wu, 2017-06-08 07:03:24]

The CQ bit was checked by donna.wu@intel.com

[commit-bot: I haz the power, 2017-06-08 07:03:44]

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-06-08 07:11:31]

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

[commit-bot: I haz the power, 2017-06-08 07:11:33]

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

[donna.wu, 2017-06-08 07:27:59]

The CQ bit was checked by donna.wu@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 07:28:19]

Dry run: CQ is trying da patch.

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

[donna.wu, 2017-06-08 07:32:29]

donna.wu@intel.com changed reviewers:
+ rohitrao@chromium.org, tsergeant@chromium.org
- rockot@chromium.org

[donna.wu, 2017-06-08 07:32:29]

tsergeant@chromium.org: Please review changes in 

rohitrao@chromium.org: Please review changes in

[commit-bot: I haz the power, 2017-06-08 07:34:04]

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

[commit-bot: I haz the power, 2017-06-08 07:34:05]

Dry run: This issue passed the CQ dry run.

[donna.wu, 2017-06-08 07:37:40]

The CQ bit was checked by donna.wu@intel.com

[commit-bot: I haz the power, 2017-06-08 07:37:55]

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-06-08 07:47:40]

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

[commit-bot: I haz the power, 2017-06-08 07:47:42]

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

[donna.wu, 2017-06-08 07:58:54]

The CQ bit was checked by donna.wu@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 07:59:06]

Dry run: CQ is trying da patch.

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

[donna.wu, 2017-06-08 07:59:17]

The CQ bit was unchecked by donna.wu@intel.com

[tsergeant, 2017-06-08 08:57:46]

rs lgtm for browsing_history_handler_unittest.cc

[rohitrao (ping after 24h), 2017-06-09 17:44:30]

ios/ lgtm

[donna.wu, 2017-06-12 02:58:33]

The CQ bit was checked by donna.wu@intel.com

[commit-bot: I haz the power, 2017-06-12 02:58:38]

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-06-12 03:57:02]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497236313546040,
"parent_rev": "8f61c09fb2b62a73f36eac10eb873690b00810b9", "commit_rev":
"370a037ae7d0cd617c45942610c5866aff44a35c"}

[commit-bot: I haz the power, 2017-06-12 03:57:12]

Description was changed from

==========
Remove ScopedVector usage from components/history

This CL replaces ScopedVector with std::vector<std::unique_ptr>
in components/history.
ScopedVector is deprecated, see bug.

BUG=554289
==========

to

==========
Remove ScopedVector usage from components/history

This CL replaces ScopedVector with std::vector<std::unique_ptr>
in components/history.
ScopedVector is deprecated, see bug.

BUG=554289

Review-Url: https://codereview.chromium.org/2906953003
Cr-Commit-Position: refs/heads/master@{#478547}
Committed:
https://chromium.googlesource.com/chromium/src/+/370a037ae7d0cd617c45942610c5...
==========

[commit-bot: I haz the power, 2017-06-12 03:57:13]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/370a037ae7d0cd617c45942610c5...