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

Issue 6352005: Don't register cookie access on empty cookie list. (Closed)

Created:
9 years, 11 months ago by jochen (gone - plz use gerrit)
Modified:
9 years, 7 months ago
CC:
chromium-reviews, Paweł Hajdan Jr., brettw-cc_chromium.org
Visibility:
Public.

Description

Don't register cookie access on empty cookie list. BUG=none TEST=TabSpecificContentSettingsTest.EmptyCookieList Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72009

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -1 line) Patch
M chrome/browser/tab_contents/tab_specific_content_settings.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/tab_contents/tab_specific_content_settings_unittest.cc View 2 chunks +18 lines, -1 line 0 comments Download

Messages

Total messages: 5 (0 generated)
jochen (gone - plz use gerrit)
Please review
9 years, 11 months ago (2011-01-18 15:16:41 UTC) #1
darin (slow to review)
LGTM
9 years, 11 months ago (2011-01-19 06:07:59 UTC) #2
darin (slow to review)
actually, can you say why you chose this solution over suppressing the OnCookiesRead notification?
9 years, 11 months ago (2011-01-19 06:09:00 UTC) #3
jochen (gone - plz use gerrit)
On 2011/01/19 06:09:00, darin wrote: > actually, can you say why you chose this solution ...
9 years, 11 months ago (2011-01-19 11:37:33 UTC) #4
darin (slow to review)
9 years, 11 months ago (2011-01-19 22:31:22 UTC) #5
OK, no need for further changes then.
-Darin

On Wed, Jan 19, 2011 at 3:37 AM, <jochen@chromium.org> wrote:

> On 2011/01/19 06:09:00, darin wrote:
>
>> actually, can you say why you chose this solution over suppressing the
>> OnCookiesRead notification?
>>
>
> We could suppress this in addition to this change, but since the interface
> doesn't allow for excluding an empty cookie list, the implementation needs
> to
> catch this case.
>
>
> http://codereview.chromium.org/6352005/
>

Powered by Google App Engine
This is Rietveld 408576698