Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(311)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 8892034: Use base::Passed(scoped_ptr) in policy/ code. (Closed)

Created:
9 years ago by Joao da Silva
Modified:
8 years, 7 months ago
Reviewers:
awong
CC:
chromium-reviews, Paweł Hajdan Jr.
Visibility:
Public.

Description

Use base::Passed(scoped_ptr) in policy/ code. BUG=None TEST=Everything works as before

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -45 lines) Patch
M chrome/browser/policy/asynchronous_policy_loader.h View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/policy/asynchronous_policy_loader.cc View 1 chunk +6 lines, -4 lines 0 comments Download
M chrome/browser/policy/url_blacklist_manager.h View 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/policy/url_blacklist_manager.cc View 3 chunks +23 lines, -34 lines 2 comments Download
M chrome/browser/policy/url_blacklist_manager_unittest.cc View 1 chunk +3 lines, -3 lines 3 comments Download

Messages

Total messages: 6 (0 generated)
Joao da Silva
Hey Albert, do you mind reviewing this little update towards using base::Passed? I stumbled upon ...
9 years ago (2011-12-10 14:11:54 UTC) #1
awong
http://codereview.chromium.org/8892034/diff/1/chrome/browser/policy/url_blacklist_manager.cc File chrome/browser/policy/url_blacklist_manager.cc (right): http://codereview.chromium.org/8892034/diff/1/chrome/browser/policy/url_blacklist_manager.cc#newcode374 chrome/browser/policy/url_blacklist_manager.cc:374: URLBlacklist* raw_blacklist = blacklist.get(); On 2011/12/10 14:11:54, Joao da ...
9 years ago (2011-12-12 22:01:26 UTC) #2
awong
http://codereview.chromium.org/8892034/diff/1/chrome/browser/policy/url_blacklist_manager_unittest.cc File chrome/browser/policy/url_blacklist_manager_unittest.cc (right): http://codereview.chromium.org/8892034/diff/1/chrome/browser/policy/url_blacklist_manager_unittest.cc#newcode52 chrome/browser/policy/url_blacklist_manager_unittest.cc:52: MOCK_METHOD1(SetBlacklist, void(URLBlacklist*)); On 2011/12/12 22:01:26, awong wrote: > On ...
9 years ago (2011-12-12 22:15:27 UTC) #3
Joao da Silva
On Mon, Dec 12, 2011 at 11:15 PM, <ajwong@chromium.org> wrote: > > http://codereview.chromium.**org/8892034/diff/1/chrome/** > browser/policy/url_blacklist_**manager_unittest.cc<http://codereview.chromium.org/8892034/diff/1/chrome/browser/policy/url_blacklist_manager_unittest.cc> ...
9 years ago (2011-12-12 23:34:45 UTC) #4
awong
:( I think we'll need to e-mail the gmock mailing list. The problem that's almost ...
9 years ago (2011-12-12 23:40:13 UTC) #5
awong
8 years, 11 months ago (2012-01-04 02:52:15 UTC) #6 
FYI, here's the start:

http://code.google.com/p/googletest/issues/detail?id=395
http://codereview.appspot.com/5504122/

But I don't know if it's going to go anywhere.

For not, it might be best to try to find another way to handle this.  Maybe
some sort of hack like

virtual void SetBlacklist(scoped_ptr<URLBlacklist> ptr) {
  SetBlacklistMock(ptr.get());
}

MOCK_METHOD1(SetBlacklistMock, void(URLBlacklist*));

And then set the expectations on SetBlacklistMock?


On Mon, Dec 12, 2011 at 3:39 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

> :(  I think we'll need to e-mail the gmock mailing list.
>
> The problem that's almost certainly occuring is that gmock internally is
> making a copy of the parameter and passing it around.  This cannot work
> with scoped_ptr.  We'd need to update gmock to use a "forward()" function
> internally on each argument in order to support this.  This would provide a
> hook for us to modify "forward" to support movable but not copyable types.
>
> This is going to be non-trivial amount of work.
>
> -Albert
>
>
>
> On Mon, Dec 12, 2011 at 3:34 PM, Joao da Silva
<joaodasilva@chromium.org>wrote:
>
>>
>>
>> On Mon, Dec 12, 2011 at 11:15 PM, <ajwong@chromium.org> wrote:
>>
>>>
>>> http://codereview.chromium.**org/8892034/diff/1/chrome/**
>>>
browser/policy/url_blacklist_**manager_unittest.cc<http://codereview.chromium.org/8892034/diff/1/chrome/browser/policy/url_blacklist_manager_unittest.cc>
>>> File chrome/browser/policy/url_**blacklist_manager_unittest.cc (right):
>>>
>>> http://codereview.chromium.**org/8892034/diff/1/chrome/**
>>>
browser/policy/url_blacklist_**manager_unittest.cc#newcode52<http://codereview.chromium.org/8892034/diff/1/chrome/browser/policy/url_blacklist_manager_unittest.cc#newcode52>
>>> chrome/browser/policy/url_**blacklist_manager_unittest.cc:**52:
>>> MOCK_METHOD1(SetBlacklist, void(URLBlacklist*));
>>> On 2011/12/12 22:01:26, awong wrote:
>>>
>>>> On 2011/12/10 14:11:54, Joao da Silva wrote:
>>>> > scoped_ptr<URLBlacklist> as an argument didn't work. I think it was
>>>>
>>> because of
>>>
>>>> > the SetBlacklist(_) form below, and scoped_ptrs not having a copy
>>>>
>>> ctor.
>>>
>>>  Huh...that's not good.  I'm not quite sure if this is actually
>>>>
>>> solvable...
>>>
>>>  Will need to ask the gmock team.
>>>>
>>>
>>> What's the specific error message you get from this?  Can you not even
>>> declare the MOCK_METHOD?  Or is it just that you can't set an
>>> expectation?
>>>
>>
>> Can't even declare the MOCK_METHOD. Removing the expectations leads to
>> the exact same build errors, which you can find attached. That's just by
>> replacing the mock line with
>>
>> MOCK_METHOD1(SetBlacklist, void(scoped_ptr<URLBlacklist>));
>>
>> I'll try building with clang tomorrow and hope for better errors :-)
>>
>>
>>>
http://codereview.chromium.**org/8892034/<http://codereview.chromium.org/8892...
>>>
>>
>>
>

Powered by Google App Engine
This is Rietveld 408576698