Adds local storage nodes to cookie tree model and cookies view.

Adds local storage nodes to cookie tree model and cookies view.

BUG=none
TEST=The show cookie dialog box should have a new node "local storage" when appropriate. When selected, it should display details of local storage (name, size on disk, last modified) in the details frame.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=37001

[dumi, 2010-01-08 23:57:56]

http://codereview.chromium.org/523139/diff/2001/28
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/523139/diff/2001/28#newcode50
chrome/browser/cookies_tree_model.h:50: local_storage_info(local_storage_info)
{}
maybe add a DCHECK(local_storage_info) if (node_type == TYPE_LOCAL_STORAGE(S)).

http://codereview.chromium.org/523139/diff/2001/28#newcode280
chrome/browser/cookies_tree_model.h:280: scoped_refptr<LocalStorageModel>
local_storage_model_;
i'm a bit confused by the hierarchy here. i thought it would be something like:

(cookies classes that we have now)
CookieTreeNode : TreeNode<CookieTreeNode>
CookieTreeXYZNode : CookieTreeNode
CookieTreeModel : TreeNodeModel<CookieTreeNode>

(new local storage classes)
LocalStorageTreeNode : TreeNode<LocalStorageNode> (base local storage node)
LocalStorageTreeLocalStoragesNode : LocalStorageTreeNode (the node that
represents the collection of all local storage files for an origin; similar to
CookeTreeCookiesNode)
LocalStorageTreeLocalStorageNode : LocalStorageTreeNode (the node that represets
one local storage file; similar to CookieTreeCookieNode)
LocalStorageTreeModel : TreeNodeModel<LocalStorageTreeNode>

and then things like local_storage_model_ would live inside the
LocalStorageTreeNode class, and would be returned by
LocalStorageTreeNode::GetModel(), and methods like DeleteLocalstorage() and
DeleteAllLocalStorage() would live in that class too.

if you want to, let's talk a bit about this. i'm going to have to do the same
thing for databases, so i wanna make sure we have similar implementations.

http://codereview.chromium.org/523139/diff/2001/29
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/523139/diff/2001/29#newcode27
chrome/browser/in_process_webkit/dom_storage_context.cc:27: FilePath old_path =
data_path.AppendASCII("localStorage");
should we have a FilePath::CharType constant for "localStorage" too?

http://codereview.chromium.org/523139/diff/2001/29#newcode185
chrome/browser/in_process_webkit/dom_storage_context.cc:185: if
(safe_file_path.Extension().find(kLocalStorageExtension) ==
why not compare safe_file_path.Extension() to kLocalStorageExtension? are
.localstorage<more_stuff_here> extensions valid too?

and if we expect this to never happen, maybe use a DCHECK instead?

http://codereview.chromium.org/523139/diff/2001/29#newcode208
chrome/browser/in_process_webkit/dom_storage_context.cc:208: if
(file_path.Extension().find(kLocalStorageExtension) !=
same comment.

http://codereview.chromium.org/523139/diff/2001/29#newcode213
chrome/browser/in_process_webkit/dom_storage_context.cc:213: for
(std::vector<FilePath>::iterator file_path = files_to_delete.begin();
minor: const_iterator?

http://codereview.chromium.org/523139/diff/2001/29#newcode215
chrome/browser/in_process_webkit/dom_storage_context.cc:215: ++file_path) {
minor: i think we usually use iterator++. i believe ++i is mostly used for
integer types only.

http://codereview.chromium.org/523139/diff/2001/29#newcode223
chrome/browser/in_process_webkit/dom_storage_context.cc:223:
CHECK(local_storage_info_out);
i think this should be a DCHECK.

http://codereview.chromium.org/523139/diff/2001/29#newcode230
chrome/browser/in_process_webkit/dom_storage_context.cc:230: if
(file_path.Extension().find(kLocalStorageExtension) !=
same extension comment.

http://codereview.chromium.org/523139/diff/2001/29#newcode235
chrome/browser/in_process_webkit/dom_storage_context.cc:235: file_util::FileInfo
file_info = {0};
this looks a bit ugly to me. i think we should either initialize file_info to
{0, 0, base::Time()}, or we should add a default FileInfo() constructor and not
initialize it here at all.

http://codereview.chromium.org/523139/diff/2001/31
File chrome/browser/local_storage_model.cc (right):

http://codereview.chromium.org/523139/diff/2001/31#newcode51
chrome/browser/local_storage_model.cc:51: CHECK(!has_finished_);
minor/optional: i feel like all CHECKs in this class should be DCHECKs, since
all these variables come from our code (vs. some third party library over which
we don't have control), which means that we can test this code and guarantee
that all these conditions are true before we release it.

personally, i would use CHECKs only for critical things such as "check that we
can open the profile directory", because if we can't open the profile directory,
then we can't do almost anything, so we might as well just crash the browser.

i don't feel strongly about this though, so it's up to you if you wanna convert
these CHECKs to DCHECKs or not.

http://codereview.chromium.org/523139/diff/2001/31#newcode73
chrome/browser/local_storage_model.cc:73: if (completion_callback_) {
no need for {} here. the chromium style for if-blocks is kinda weird (imho):

// all conditions on 1-line, 1-line block ==> do not use {}
if (condition)
  do_something();

// conditions on multiple lines ==> use {}
if (condition1 &&
    condition2) {
  do_something();
}

// comments count as a line too
if (condition) {
  // comment
  do_something();
}

also, if you have if-else blocks and at least one branch uses {}, then all other
branches must use {} too:

if (condition)
  do_something();
else
  do_something_else();

if (condition) {
  do_something1();
  do_something2();
} else {  // use {}, even though it's a 1-line block
  do_something_else();
}

http://codereview.chromium.org/523139/diff/2001/23
File chrome/browser/local_storage_model_unittest.cc (right):

http://codereview.chromium.org/523139/diff/2001/23#newcode65
chrome/browser/local_storage_model_unittest.cc:65: ASSERT_EQ(2,
local_storage_info.size());
you're gonna get a warning here on mac and linux, because std::vector::size()'s
return type is size_t (unsigned) and 2 is signed. you should use
ASSERT_EQ(size_t(2), blah.size()).

http://codereview.chromium.org/523139/diff/2001/23#newcode162
chrome/browser/local_storage_model_unittest.cc:162:
ASSERT_EQ(FilePath(kTestFileInvalid).value(), file_path.BaseName().value());
FilePath overwrites operator==, so i think you should replace this with
ASSERT_EQ(FilePath(kTestFileInvalid), file_path.BaseName()). the advantage of
using FilePath::operator== is that on windows it (correctly!) ignores letter
cases when comparing two paths.

http://codereview.chromium.org/523139/diff/2001/24
File chrome/browser/views/options/cookies_view.cc (right):

http://codereview.chromium.org/523139/diff/2001/24#newcode605
chrome/browser/views/options/cookies_view.cc:605: if
(!cookies_tree_model_.get()->GetRoot()->GetChildCount()) {
no need for {}, see my other comment about if-else blocks.

[bulach_, 2010-01-09 01:06:47]

many thanks Dumitru, very helpful comments!

I added the GTK files now (not yet finished though, I need to figure out some
minor layout glitches), addressed almost all your comments plus a few questions
on how to proceed with the tree refactoring.

would you mind another look please?

http://codereview.chromium.org/523139/diff/2001/28
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/523139/diff/2001/28#newcode50
chrome/browser/cookies_tree_model.h:50: local_storage_info(local_storage_info)
{}
On 2010/01/08 23:57:56, dumi wrote:
> maybe add a DCHECK(local_storage_info) if (node_type ==
TYPE_LOCAL_STORAGE(S)).

Done.

http://codereview.chromium.org/523139/diff/2001/28#newcode280
chrome/browser/cookies_tree_model.h:280: scoped_refptr<LocalStorageModel>
local_storage_model_;
On 2010/01/08 23:57:56, dumi wrote:
> i'm a bit confused by the hierarchy here. i thought it would be something
like:
> 
> (cookies classes that we have now)
> CookieTreeNode : TreeNode<CookieTreeNode>
> CookieTreeXYZNode : CookieTreeNode
> CookieTreeModel : TreeNodeModel<CookieTreeNode>
> 
> (new local storage classes)
> LocalStorageTreeNode : TreeNode<LocalStorageNode> (base local storage node)
> LocalStorageTreeLocalStoragesNode : LocalStorageTreeNode (the node that
> represents the collection of all local storage files for an origin; similar to
> CookeTreeCookiesNode)
> LocalStorageTreeLocalStorageNode : LocalStorageTreeNode (the node that
represets
> one local storage file; similar to CookieTreeCookieNode)
> LocalStorageTreeModel : TreeNodeModel<LocalStorageTreeNode>
> 
> and then things like local_storage_model_ would live inside the
> LocalStorageTreeNode class, and would be returned by
> LocalStorageTreeNode::GetModel(), and methods like DeleteLocalstorage() and
> DeleteAllLocalStorage() would live in that class too.
> 
> if you want to, let's talk a bit about this. i'm going to have to do the same
> thing for databases, so i wanna make sure we have similar implementations.

yeah, we took me a while to understand this bit as well.. :)
I asked Jeremy, and iirc the idea was:
CookieTreeModel: the "root" tree for all types, i.e., it'll contain cookies
(hence the existing dup. on 'Cookie'Tree'Cookies'Node), local storage and DBs..
I agree it's confusing, and I'd be happy to refactor!

how do you guys prefer?
refactor before in preparation for this change, then introduce new
functionality?
introduce new functionality, see how it goes / what needs to be done, then do
the refactoring?
all at once so we can see the big picture?

independently on how we go, I think it'd be useful to have a common prefix to
group these, probably deriving from the model name? perhaps: 
LocalDataModel
  LocalDataOriginNode
    LocalDataCookieTreeNode
    LocalDataLocalStorageNode
    ...
    LocalDataDBNode

I'm open to suggestions for better naming!

http://codereview.chromium.org/523139/diff/2001/29
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/523139/diff/2001/29#newcode27
chrome/browser/in_process_webkit/dom_storage_context.cc:27: FilePath old_path =
data_path.AppendASCII("localStorage");
On 2010/01/08 23:57:56, dumi wrote:
> should we have a FilePath::CharType constant for "localStorage" too?

Done.

http://codereview.chromium.org/523139/diff/2001/29#newcode185
chrome/browser/in_process_webkit/dom_storage_context.cc:185: if
(safe_file_path.Extension().find(kLocalStorageExtension) ==
On 2010/01/08 23:57:56, dumi wrote:
> why not compare safe_file_path.Extension() to kLocalStorageExtension? are
> .localstorage<more_stuff_here> extensions valid too?
> 
> and if we expect this to never happen, maybe use a DCHECK instead?

very good point, done!

http://codereview.chromium.org/523139/diff/2001/29#newcode208
chrome/browser/in_process_webkit/dom_storage_context.cc:208: if
(file_path.Extension().find(kLocalStorageExtension) !=
On 2010/01/08 23:57:56, dumi wrote:
> same comment.

Done.

http://codereview.chromium.org/523139/diff/2001/29#newcode213
chrome/browser/in_process_webkit/dom_storage_context.cc:213: for
(std::vector<FilePath>::iterator file_path = files_to_delete.begin();
On 2010/01/08 23:57:56, dumi wrote:
> minor: const_iterator?

Done.

http://codereview.chromium.org/523139/diff/2001/29#newcode215
chrome/browser/in_process_webkit/dom_storage_context.cc:215: ++file_path) {
On 2010/01/08 23:57:56, dumi wrote:
> minor: i think we usually use iterator++. i believe ++i is mostly used for
> integer types only.

Done.

http://codereview.chromium.org/523139/diff/2001/29#newcode223
chrome/browser/in_process_webkit/dom_storage_context.cc:223:
CHECK(local_storage_info_out);
On 2010/01/08 23:57:56, dumi wrote:
> i think this should be a DCHECK.

Done (just so to explain the rationale we had in my previous team, and ask how
we do things here:
All things that could go horribly wrong, i.e., deref a pointer, we'd use the
equivalent of CHECK.. recoverable errors we'd go with a DCHECK. obviously,
there's never a clear cut and the debate never ended :) but I'd be interested to
know what's the general policy around here?)

http://codereview.chromium.org/523139/diff/2001/29#newcode230
chrome/browser/in_process_webkit/dom_storage_context.cc:230: if
(file_path.Extension().find(kLocalStorageExtension) !=
On 2010/01/08 23:57:56, dumi wrote:
> same extension comment.

Done.

http://codereview.chromium.org/523139/diff/2001/29#newcode235
chrome/browser/in_process_webkit/dom_storage_context.cc:235: file_util::FileInfo
file_info = {0};
On 2010/01/08 23:57:56, dumi wrote:
> this looks a bit ugly to me. i think we should either initialize file_info to
> {0, 0, base::Time()}, or we should add a default FileInfo() constructor and
not
> initialize it here at all.

sorry, I should've at least DCHECK'd GetFileInfo return. done.

http://codereview.chromium.org/523139/diff/2001/31
File chrome/browser/local_storage_model.cc (right):

http://codereview.chromium.org/523139/diff/2001/31#newcode51
chrome/browser/local_storage_model.cc:51: CHECK(!has_finished_);
On 2010/01/08 23:57:56, dumi wrote:
> minor/optional: i feel like all CHECKs in this class should be DCHECKs, since
> all these variables come from our code (vs. some third party library over
which
> we don't have control), which means that we can test this code and guarantee
> that all these conditions are true before we release it.
> 
> personally, i would use CHECKs only for critical things such as "check that we
> can open the profile directory", because if we can't open the profile
directory,
> then we can't do almost anything, so we might as well just crash the browser.
> 
> i don't feel strongly about this though, so it's up to you if you wanna
convert
> these CHECKs to DCHECKs or not.

I'm happy with your suggestion, changed all to DCHECK.
btw, very good point about our code vs third party, it surely helps when
choosing check over dcheck.

http://codereview.chromium.org/523139/diff/2001/31#newcode73
chrome/browser/local_storage_model.cc:73: if (completion_callback_) {
On 2010/01/08 23:57:56, dumi wrote:
> no need for {} here. the chromium style for if-blocks is kinda weird (imho):
> 
> // all conditions on 1-line, 1-line block ==> do not use {}
> if (condition)
>   do_something();
> 
> // conditions on multiple lines ==> use {}
> if (condition1 &&
>     condition2) {
>   do_something();
> }
> 
> // comments count as a line too
> if (condition) {
>   // comment
>   do_something();
> }
> 
> also, if you have if-else blocks and at least one branch uses {}, then all
other
> branches must use {} too:
> 
> if (condition)
>   do_something();
> else
>   do_something_else();
> 
> if (condition) {
>   do_something1();
>   do_something2();
> } else {  // use {}, even though it's a 1-line block
>   do_something_else();
> }
> 

thanks for the detailed info! I'll add this style to my buffer and use it
appropriately (so many styles, so many string types!) :)

http://codereview.chromium.org/523139/diff/2001/23
File chrome/browser/local_storage_model_unittest.cc (right):

http://codereview.chromium.org/523139/diff/2001/23#newcode65
chrome/browser/local_storage_model_unittest.cc:65: ASSERT_EQ(2,
local_storage_info.size());
On 2010/01/08 23:57:56, dumi wrote:
> you're gonna get a warning here on mac and linux, because
std::vector::size()'s
> return type is size_t (unsigned) and 2 is signed. you should use
> ASSERT_EQ(size_t(2), blah.size()).

Done.

http://codereview.chromium.org/523139/diff/2001/23#newcode162
chrome/browser/local_storage_model_unittest.cc:162:
ASSERT_EQ(FilePath(kTestFileInvalid).value(), file_path.BaseName().value());
On 2010/01/08 23:57:56, dumi wrote:
> FilePath overwrites operator==, so i think you should replace this with
> ASSERT_EQ(FilePath(kTestFileInvalid), file_path.BaseName()). the advantage of
> using FilePath::operator== is that on windows it (correctly!) ignores letter
> cases when comparing two paths.

done! however, had to use ASSERT_TRUE, gtest doesn't seem too happy with using
operator==..

http://codereview.chromium.org/523139/diff/2001/24
File chrome/browser/views/options/cookies_view.cc (right):

http://codereview.chromium.org/523139/diff/2001/24#newcode605
chrome/browser/views/options/cookies_view.cc:605: if
(!cookies_tree_model_.get()->GetRoot()->GetChildCount()) {
On 2010/01/08 23:57:56, dumi wrote:
> no need for {}, see my other comment about if-else blocks.

Done.

[dumi, 2010-01-09 02:49:38]

> yeah, we took me a while to understand this bit as well.. :)
> I asked Jeremy, and iirc the idea was:
> CookieTreeModel: the "root" tree for all types, i.e., it'll contain cookies
> (hence the existing dup. on 'Cookie'Tree'Cookies'Node), local storage and
DBs..
> I agree it's confusing, and I'd be happy to refactor!
> 
> how do you guys prefer?
> refactor before in preparation for this change, then introduce new
> functionality?
> introduce new functionality, see how it goes / what needs to be done, then do
> the refactoring?
> all at once so we can see the big picture?
> 
> independently on how we go, I think it'd be useful to have a common prefix to
> group these, probably deriving from the model name? perhaps: 
> LocalDataModel
>   LocalDataOriginNode
>     LocalDataCookieTreeNode
>     LocalDataLocalStorageNode
>     ...
>     LocalDataDBNode
> 
> I'm open to suggestions for better naming!

i think this might be a good time to refactor a bit this code. here's the way
i'm seeing this tree (current class names in parenthesis):

('Data' prefix is probably a bad choice/unnecessary)
DataTreeNode(CookieTreeNode): base class for all tree nodes

DataTreeRootNode(CookieTreeRootNode): the root node (invisible in the UI)
     DataTreeOriginNode(CookieTreeOriginNode): a node representing an origin
          DataTreeCookiesNode(CookieTreeCookiesNode): the "Cookies" label
               DataTreeCookieNode(CookieTreeCookieNode): shows 1 cookie
          DataTreeLocalStoragesNode: the "Local Storage" label
               DataTreeLocalStorageNode: shows 1 local storage file
          DataTreeDatabasesNode: the "Databases" label
               DataTreeDatabaseNode: shows 1 DB

then we'd also have 3 model classes (all inheriting from TreeNodeModel):
1. CookiesTreeModel: cookies-specific methods, data, etc. each
DataTreeCookiesNode would have an instance of this class (or they could all
probably share the same CookiesTreeModel instance too).
2. LocalStoragesTreeModel: similar, but for local storage
3. DatabasesTreeModel: similar, but for DBs

jeremy, marcus, what do you think? i'd be happy to refactor this code quickly,
or review it if marcus wants to do it (in a separate CL that should come before
the local storage additions?).

[jorlow, 2010-01-09 03:01:37]

+ Ian and UI team (for final approval of this change)

From what I know, the proposed refactoring sounds good...though Ian might have
some thoughts as well.

I'll look over this from a LocalStorage perspective soon, but I think Dumi's got
the rest of the review well covered at this point.

Whatever you guys decide about who does the refactoring, please make sure that
it doesn't take up too much of Marcus' time next week.  I want to go over some
WebKit stuff with him since he only has one more week in MTV.

[dumi, 2010-01-09 03:46:09]

sounds good.

marcus, then you should probably go ahead with your change (i'll take
another look at your CL), and i can do the refactoring (if any) after that.
or if i manage to get it in before your change, i'll help you merge them.

dumi


On Fri, Jan 8, 2010 at 7:01 PM, <jorlow@chromium.org> wrote:

> + Ian and UI team (for final approval of this change)
>
> From what I know, the proposed refactoring sounds good...though Ian might
> have
> some thoughts as well.
>
> I'll look over this from a LocalStorage perspective soon, but I think
> Dumi's got
> the rest of the review well covered at this point.
>
> Whatever you guys decide about who does the refactoring, please make sure
> that
> it doesn't take up too much of Marcus' time next week.  I want to go over
> some
> WebKit stuff with him since he only has one more week in MTV.
>
>
> http://codereview.chromium.org/523139
>

[jorlow, 2010-01-09 04:20:49]

http://codereview.chromium.org/523139/diff/6001/7006
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/523139/diff/6001/7006#newcode51
chrome/browser/cookies_tree_model.h:51: if (node_type == TYPE_LOCAL_STORAGE)
DCHECK(local_storage_info);
split lines

http://codereview.chromium.org/523139/diff/6001/7007
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/523139/diff/6001/7007#newcode182
chrome/browser/in_process_webkit/dom_storage_context.cc:182: PurgeMemory();
Create a bug about purging only what's needed, assign it to me, and add a TODO
about it....it's not a big enough deal to do it now tho.

http://codereview.chromium.org/523139/diff/6001/7007#newcode184
chrome/browser/in_process_webkit/dom_storage_context.cc:184: // Rather than just
delete the passed file, let's re-construct its path.
Why?

http://codereview.chromium.org/523139/diff/6001/7007#newcode207
chrome/browser/in_process_webkit/dom_storage_context.cc:207:
std::vector<FilePath> files_to_delete;
Why do it this way?  Does the enumerator get screwed up if the directory
changes?  It'd surprise me if so.

http://codereview.chromium.org/523139/diff/6001/7007#newcode217
chrome/browser/in_process_webkit/dom_storage_context.cc:217: file_path++) {
++blah not blah++

http://codereview.chromium.org/523139/diff/6001/7007#newcode223
chrome/browser/in_process_webkit/dom_storage_context.cc:223:
std::vector<LocalStorageInfo>* local_storage_info_out) {
I don't think you need _out.

http://codereview.chromium.org/523139/diff/6001/7007#newcode228
chrome/browser/in_process_webkit/dom_storage_context.cc:228: false,
combine these lines

http://codereview.chromium.org/523139/diff/6001/7007#newcode237
chrome/browser/in_process_webkit/dom_storage_context.cc:237: bool ret =
file_util::GetFileInfo(file_path, &file_info);
I think you can get all of this information from the FindInfo that's free when
you use the enumerator.  Check out DeleteDataModifiedSince above.

http://codereview.chromium.org/523139/diff/6001/7008
File chrome/browser/in_process_webkit/dom_storage_context.h (right):

http://codereview.chromium.org/523139/diff/6001/7008#newcode29
chrome/browser/in_process_webkit/dom_storage_context.h:29: struct
LocalStorageInfo {
This should really be in the same file as the model.

http://codereview.chromium.org/523139/diff/6001/7009
File chrome/browser/local_storage_model.cc (right):

http://codereview.chromium.org/523139/diff/6001/7009#newcode16
chrome/browser/local_storage_model.cc:16: void
LocalStorageModel::StartFetching(Callback0::Type* callback) {
Use Task* instead.

Or....I think there's some standard type for a function/method with unbound
arguments.  It might be possible to do that rather than having the
GetLocalStorageInfo method?

http://codereview.chromium.org/523139/diff/6001/7009#newcode17
chrome/browser/local_storage_model.cc:17:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
Check the has_finished here..and elsewhere maybe?

Also verify the completion callback is currently null.

http://codereview.chromium.org/523139/diff/6001/7009#newcode30
chrome/browser/local_storage_model.cc:30: void
LocalStorageModel::DeleteFile(const FilePath& file_path) {
Function order here should generally match that of the .h file.

http://codereview.chromium.org/523139/diff/6001/7009#newcode61
chrome/browser/local_storage_model.cc:61:
std::vector<DOMStorageContext::LocalStorageInfo>* local_storage_info_out) {
_out seems redundant since this takes a non-const pointer, I think it's obvious?
 Or maybe the style guide says you should use out here?

If you change it, change it in the .h file too.

http://codereview.chromium.org/523139/diff/6001/7009#newcode73
chrome/browser/local_storage_model.cc:73: if (completion_callback_)
completion_callback_->Run();
normally we don't put if statements on one line even if it's possible...it's
just easier to read

http://codereview.chromium.org/523139/diff/6001/7004
File chrome/browser/local_storage_model.h (right):

http://codereview.chromium.org/523139/diff/6001/7004#newcode15
chrome/browser/local_storage_model.h:15: // This class fetches local storage
information in the File thread, and notifies
WebKit thread

http://codereview.chromium.org/523139/diff/6001/7004#newcode16
chrome/browser/local_storage_model.h:16: // the UI thread upon completion.
I feel like you should go into a bit more detail.

http://codereview.chromium.org/523139/diff/6001/7004#newcode50
chrome/browser/local_storage_model.h:50: DOMStorageContext*
dom_storage_context_;
const?

http://codereview.chromium.org/523139/diff/6001/7004#newcode52
chrome/browser/local_storage_model.h:52: Callback0::Type* completion_callback_;
Task* ?

http://codereview.chromium.org/523139/diff/6001/7001
File chrome/browser/local_storage_model_unittest.cc (right):

http://codereview.chromium.org/523139/diff/6001/7001#newcode35
chrome/browser/local_storage_model_unittest.cc:35: static const
FilePath::CharType* kFilesToCreate[] = {
Probably no need for this to be static.

http://codereview.chromium.org/523139/diff/6001/7001#newcode45
chrome/browser/local_storage_model_unittest.cc:45: FilePath
storage_path(testing_profile_.GetPath());
We now have a couple places where we calculate the local storage
directory....maybe we should add a function to the profile to do it for us?  
(Is there precedent?)

http://codereview.chromium.org/523139/diff/6001/7012
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/523139/diff/6001/7012#newcode182
chrome/test/testing_profile.h:182: virtual WebKitContext* GetWebKitContext() {
IIRC...Isn't this the same as the default...in which case we should just delete
this function?

http://codereview.chromium.org/523139/diff/6001/7012#newcode254
chrome/test/testing_profile.h:254: // WebKitContext, lazily initialized by
GetWebKitContext().
If so, we don't need this.

[bulach_, 2010-01-11 18:12:01]

thanks for all the many helpful comments!
I changed a few things as you suggested, it looks better now.
there a few minor questions remaining below.

as for the renaming: I'm happy to do, but I think it's orthogonal to this
specific change, so we could either do it before or after this one.. what do you
think? any suggestions for name?

dumi suggested Data. how about LocalData, or even ClientData, since this will
contain cookies, local storage, databases, ...?

we'll probably need to rename the .cc files, as well as the buttons / windows
title, etc..


On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> http://codereview.chromium.org/523139/diff/6001/7006
> File chrome/browser/cookies_tree_model.h (right):
> 
> http://codereview.chromium.org/523139/diff/6001/7006#newcode51
> chrome/browser/cookies_tree_model.h:51: if (node_type == TYPE_LOCAL_STORAGE)
> DCHECK(local_storage_info);
> split lines
> 
> http://codereview.chromium.org/523139/diff/6001/7007
> File chrome/browser/in_process_webkit/dom_storage_context.cc (right):
> 
> http://codereview.chromium.org/523139/diff/6001/7007#newcode182
> chrome/browser/in_process_webkit/dom_storage_context.cc:182: PurgeMemory();
> Create a bug about purging only what's needed, assign it to me, and add a TODO
> about it....it's not a big enough deal to do it now tho.
> 
> http://codereview.chromium.org/523139/diff/6001/7007#newcode184
> chrome/browser/in_process_webkit/dom_storage_context.cc:184: // Rather than
just
> delete the passed file, let's re-construct its path.
> Why?
> 
> http://codereview.chromium.org/523139/diff/6001/7007#newcode207
> chrome/browser/in_process_webkit/dom_storage_context.cc:207:
> std::vector<FilePath> files_to_delete;
> Why do it this way?  Does the enumerator get screwed up if the directory
> changes?  It'd surprise me if so.
> 
> http://codereview.chromium.org/523139/diff/6001/7007#newcode217
> chrome/browser/in_process_webkit/dom_storage_context.cc:217: file_path++) {
> ++blah not blah++
> 
> http://codereview.chromium.org/523139/diff/6001/7007#newcode223
> chrome/browser/in_process_webkit/dom_storage_context.cc:223:
> std::vector<LocalStorageInfo>* local_storage_info_out) {
> I don't think you need _out.
> 
> http://codereview.chromium.org/523139/diff/6001/7007#newcode228
> chrome/browser/in_process_webkit/dom_storage_context.cc:228: false,
> combine these lines
> 
> http://codereview.chromium.org/523139/diff/6001/7007#newcode237
> chrome/browser/in_process_webkit/dom_storage_context.cc:237: bool ret =
> file_util::GetFileInfo(file_path, &file_info);
> I think you can get all of this information from the FindInfo that's free when
> you use the enumerator.  Check out DeleteDataModifiedSince above.
> 
> http://codereview.chromium.org/523139/diff/6001/7008
> File chrome/browser/in_process_webkit/dom_storage_context.h (right):
> 
> http://codereview.chromium.org/523139/diff/6001/7008#newcode29
> chrome/browser/in_process_webkit/dom_storage_context.h:29: struct
> LocalStorageInfo {
> This should really be in the same file as the model.
> 
> http://codereview.chromium.org/523139/diff/6001/7009
> File chrome/browser/local_storage_model.cc (right):
> 
> http://codereview.chromium.org/523139/diff/6001/7009#newcode16
> chrome/browser/local_storage_model.cc:16: void
> LocalStorageModel::StartFetching(Callback0::Type* callback) {
> Use Task* instead.
> 
> Or....I think there's some standard type for a function/method with unbound
> arguments.  It might be possible to do that rather than having the
> GetLocalStorageInfo method?
> 
> http://codereview.chromium.org/523139/diff/6001/7009#newcode17
> chrome/browser/local_storage_model.cc:17:
> DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
> Check the has_finished here..and elsewhere maybe?
> 
> Also verify the completion callback is currently null.
> 
> http://codereview.chromium.org/523139/diff/6001/7009#newcode30
> chrome/browser/local_storage_model.cc:30: void
> LocalStorageModel::DeleteFile(const FilePath& file_path) {
> Function order here should generally match that of the .h file.
> 
> http://codereview.chromium.org/523139/diff/6001/7009#newcode61
> chrome/browser/local_storage_model.cc:61:
> std::vector<DOMStorageContext::LocalStorageInfo>* local_storage_info_out) {
> _out seems redundant since this takes a non-const pointer, I think it's
obvious?
>  Or maybe the style guide says you should use out here?
> 
> If you change it, change it in the .h file too.
> 
> http://codereview.chromium.org/523139/diff/6001/7009#newcode73
> chrome/browser/local_storage_model.cc:73: if (completion_callback_)
> completion_callback_->Run();
> normally we don't put if statements on one line even if it's possible...it's
> just easier to read
> 
> http://codereview.chromium.org/523139/diff/6001/7004
> File chrome/browser/local_storage_model.h (right):
> 
> http://codereview.chromium.org/523139/diff/6001/7004#newcode15
> chrome/browser/local_storage_model.h:15: // This class fetches local storage
> information in the File thread, and notifies
> WebKit thread
> 
> http://codereview.chromium.org/523139/diff/6001/7004#newcode16
> chrome/browser/local_storage_model.h:16: // the UI thread upon completion.
> I feel like you should go into a bit more detail.
> 
> http://codereview.chromium.org/523139/diff/6001/7004#newcode50
> chrome/browser/local_storage_model.h:50: DOMStorageContext*
> dom_storage_context_;
> const?
> 
> http://codereview.chromium.org/523139/diff/6001/7004#newcode52
> chrome/browser/local_storage_model.h:52: Callback0::Type*
completion_callback_;
> Task* ?
> 
> http://codereview.chromium.org/523139/diff/6001/7001
> File chrome/browser/local_storage_model_unittest.cc (right):
> 
> http://codereview.chromium.org/523139/diff/6001/7001#newcode35
> chrome/browser/local_storage_model_unittest.cc:35: static const
> FilePath::CharType* kFilesToCreate[] = {
> Probably no need for this to be static.
> 
> http://codereview.chromium.org/523139/diff/6001/7001#newcode45
> chrome/browser/local_storage_model_unittest.cc:45: FilePath
> storage_path(testing_profile_.GetPath());
> We now have a couple places where we calculate the local storage
> directory....maybe we should add a function to the profile to do it for us?  
> (Is there precedent?)
> 
> http://codereview.chromium.org/523139/diff/6001/7012
> File chrome/test/testing_profile.h (right):
> 
> http://codereview.chromium.org/523139/diff/6001/7012#newcode182
> chrome/test/testing_profile.h:182: virtual WebKitContext* GetWebKitContext() {
> IIRC...Isn't this the same as the default...in which case we should just
delete
> this function?
> 
> http://codereview.chromium.org/523139/diff/6001/7012#newcode254
> chrome/test/testing_profile.h:254: // WebKitContext, lazily initialized by
> GetWebKitContext().
> If so, we don't need this.

[bulach_, 2010-01-11 20:40:45]

as discussed, uploaded a new snapshot renaming local_storage_model to
browsing_data_local_storage_fetcher..

On 2010/01/11 18:12:01, bulach wrote:
> thanks for all the many helpful comments!
> I changed a few things as you suggested, it looks better now.
> there a few minor questions remaining below.
> 
> as for the renaming: I'm happy to do, but I think it's orthogonal to this
> specific change, so we could either do it before or after this one.. what do
you
> think? any suggestions for name?
> 
> dumi suggested Data. how about LocalData, or even ClientData, since this will
> contain cookies, local storage, databases, ...?
> 
> we'll probably need to rename the .cc files, as well as the buttons / windows
> title, etc..
> 
> 
> On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > http://codereview.chromium.org/523139/diff/6001/7006
> > File chrome/browser/cookies_tree_model.h (right):
> > 
> > http://codereview.chromium.org/523139/diff/6001/7006#newcode51
> > chrome/browser/cookies_tree_model.h:51: if (node_type == TYPE_LOCAL_STORAGE)
> > DCHECK(local_storage_info);
> > split lines
> > 
> > http://codereview.chromium.org/523139/diff/6001/7007
> > File chrome/browser/in_process_webkit/dom_storage_context.cc (right):
> > 
> > http://codereview.chromium.org/523139/diff/6001/7007#newcode182
> > chrome/browser/in_process_webkit/dom_storage_context.cc:182: PurgeMemory();
> > Create a bug about purging only what's needed, assign it to me, and add a
TODO
> > about it....it's not a big enough deal to do it now tho.
> > 
> > http://codereview.chromium.org/523139/diff/6001/7007#newcode184
> > chrome/browser/in_process_webkit/dom_storage_context.cc:184: // Rather than
> just
> > delete the passed file, let's re-construct its path.
> > Why?
> > 
> > http://codereview.chromium.org/523139/diff/6001/7007#newcode207
> > chrome/browser/in_process_webkit/dom_storage_context.cc:207:
> > std::vector<FilePath> files_to_delete;
> > Why do it this way?  Does the enumerator get screwed up if the directory
> > changes?  It'd surprise me if so.
> > 
> > http://codereview.chromium.org/523139/diff/6001/7007#newcode217
> > chrome/browser/in_process_webkit/dom_storage_context.cc:217: file_path++) {
> > ++blah not blah++
> > 
> > http://codereview.chromium.org/523139/diff/6001/7007#newcode223
> > chrome/browser/in_process_webkit/dom_storage_context.cc:223:
> > std::vector<LocalStorageInfo>* local_storage_info_out) {
> > I don't think you need _out.
> > 
> > http://codereview.chromium.org/523139/diff/6001/7007#newcode228
> > chrome/browser/in_process_webkit/dom_storage_context.cc:228: false,
> > combine these lines
> > 
> > http://codereview.chromium.org/523139/diff/6001/7007#newcode237
> > chrome/browser/in_process_webkit/dom_storage_context.cc:237: bool ret =
> > file_util::GetFileInfo(file_path, &file_info);
> > I think you can get all of this information from the FindInfo that's free
when
> > you use the enumerator.  Check out DeleteDataModifiedSince above.
> > 
> > http://codereview.chromium.org/523139/diff/6001/7008
> > File chrome/browser/in_process_webkit/dom_storage_context.h (right):
> > 
> > http://codereview.chromium.org/523139/diff/6001/7008#newcode29
> > chrome/browser/in_process_webkit/dom_storage_context.h:29: struct
> > LocalStorageInfo {
> > This should really be in the same file as the model.
> > 
> > http://codereview.chromium.org/523139/diff/6001/7009
> > File chrome/browser/local_storage_model.cc (right):
> > 
> > http://codereview.chromium.org/523139/diff/6001/7009#newcode16
> > chrome/browser/local_storage_model.cc:16: void
> > LocalStorageModel::StartFetching(Callback0::Type* callback) {
> > Use Task* instead.
> > 
> > Or....I think there's some standard type for a function/method with unbound
> > arguments.  It might be possible to do that rather than having the
> > GetLocalStorageInfo method?
> > 
> > http://codereview.chromium.org/523139/diff/6001/7009#newcode17
> > chrome/browser/local_storage_model.cc:17:
> > DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
> > Check the has_finished here..and elsewhere maybe?
> > 
> > Also verify the completion callback is currently null.
> > 
> > http://codereview.chromium.org/523139/diff/6001/7009#newcode30
> > chrome/browser/local_storage_model.cc:30: void
> > LocalStorageModel::DeleteFile(const FilePath& file_path) {
> > Function order here should generally match that of the .h file.
> > 
> > http://codereview.chromium.org/523139/diff/6001/7009#newcode61
> > chrome/browser/local_storage_model.cc:61:
> > std::vector<DOMStorageContext::LocalStorageInfo>* local_storage_info_out) {
> > _out seems redundant since this takes a non-const pointer, I think it's
> obvious?
> >  Or maybe the style guide says you should use out here?
> > 
> > If you change it, change it in the .h file too.
> > 
> > http://codereview.chromium.org/523139/diff/6001/7009#newcode73
> > chrome/browser/local_storage_model.cc:73: if (completion_callback_)
> > completion_callback_->Run();
> > normally we don't put if statements on one line even if it's possible...it's
> > just easier to read
> > 
> > http://codereview.chromium.org/523139/diff/6001/7004
> > File chrome/browser/local_storage_model.h (right):
> > 
> > http://codereview.chromium.org/523139/diff/6001/7004#newcode15
> > chrome/browser/local_storage_model.h:15: // This class fetches local storage
> > information in the File thread, and notifies
> > WebKit thread
> > 
> > http://codereview.chromium.org/523139/diff/6001/7004#newcode16
> > chrome/browser/local_storage_model.h:16: // the UI thread upon completion.
> > I feel like you should go into a bit more detail.
> > 
> > http://codereview.chromium.org/523139/diff/6001/7004#newcode50
> > chrome/browser/local_storage_model.h:50: DOMStorageContext*
> > dom_storage_context_;
> > const?
> > 
> > http://codereview.chromium.org/523139/diff/6001/7004#newcode52
> > chrome/browser/local_storage_model.h:52: Callback0::Type*
> completion_callback_;
> > Task* ?
> > 
> > http://codereview.chromium.org/523139/diff/6001/7001
> > File chrome/browser/local_storage_model_unittest.cc (right):
> > 
> > http://codereview.chromium.org/523139/diff/6001/7001#newcode35
> > chrome/browser/local_storage_model_unittest.cc:35: static const
> > FilePath::CharType* kFilesToCreate[] = {
> > Probably no need for this to be static.
> > 
> > http://codereview.chromium.org/523139/diff/6001/7001#newcode45
> > chrome/browser/local_storage_model_unittest.cc:45: FilePath
> > storage_path(testing_profile_.GetPath());
> > We now have a couple places where we calculate the local storage
> > directory....maybe we should add a function to the profile to do it for us? 

> > (Is there precedent?)
> > 
> > http://codereview.chromium.org/523139/diff/6001/7012
> > File chrome/test/testing_profile.h (right):
> > 
> > http://codereview.chromium.org/523139/diff/6001/7012#newcode182
> > chrome/test/testing_profile.h:182: virtual WebKitContext* GetWebKitContext()
{
> > IIRC...Isn't this the same as the default...in which case we should just
> delete
> > this function?
> > 
> > http://codereview.chromium.org/523139/diff/6001/7012#newcode254
> > chrome/test/testing_profile.h:254: // WebKitContext, lazily initialized by
> > GetWebKitContext().
> > If so, we don't need this.

[bulach_, 2010-01-11 20:49:01]

sorry, just hit reply and forgot to publish my replies.

http://codereview.chromium.org/523139/diff/6001/7006
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/523139/diff/6001/7006#newcode51
chrome/browser/cookies_tree_model.h:51: if (node_type == TYPE_LOCAL_STORAGE)
DCHECK(local_storage_info);
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> split lines

Done.

http://codereview.chromium.org/523139/diff/6001/7007
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/523139/diff/6001/7007#newcode182
chrome/browser/in_process_webkit/dom_storage_context.cc:182: PurgeMemory();
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> Create a bug about purging only what's needed, assign it to me, and add a TODO
> about it....it's not a big enough deal to do it now tho.

added a TODO in my name, will get back to it soon.. DeleteDataModifiedSince
above could also do with a PurgeMemoryForDatabase(foo)..
on crbug.com, I couldn't find any appropriate template, is "defect report from
user" a catch-all to use in this case?

http://codereview.chromium.org/523139/diff/6001/7007#newcode184
chrome/browser/in_process_webkit/dom_storage_context.cc:184: // Rather than just
delete the passed file, let's re-construct its path.
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> Why?

hmm. I think I'm being overzealous, but since it's a public method, does it make
sense to ensure it'll only ever delete files inside its associated directory?
file_path could be anything at this point... but anyway, I agree it's probably
pointless, I'm happy to just deletefile.

http://codereview.chromium.org/523139/diff/6001/7007#newcode207
chrome/browser/in_process_webkit/dom_storage_context.cc:207:
std::vector<FilePath> files_to_delete;
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> Why do it this way?  Does the enumerator get screwed up if the directory
> changes?  It'd surprise me if so.

agree, the enumerator should be immutable.. done.

http://codereview.chromium.org/523139/diff/6001/7007#newcode217
chrome/browser/in_process_webkit/dom_storage_context.cc:217: file_path++) {
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> ++blah not blah++

so dumi said to use blah++ for iterators and ++blah for PODs.. this specific
code here has been removed, but what's the guideline for future reference?

http://codereview.chromium.org/523139/diff/6001/7007#newcode223
chrome/browser/in_process_webkit/dom_storage_context.cc:223:
std::vector<LocalStorageInfo>* local_storage_info_out) {
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> I don't think you need _out.

thanks to your suggestion, I changed it considerably and now it's part of
LocalStorageModel, there's no longer an "_out" param in there..

http://codereview.chromium.org/523139/diff/6001/7007#newcode228
chrome/browser/in_process_webkit/dom_storage_context.cc:228: false,
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> combine these lines

done in local_storage_model.cc

http://codereview.chromium.org/523139/diff/6001/7007#newcode237
chrome/browser/in_process_webkit/dom_storage_context.cc:237: bool ret =
file_util::GetFileInfo(file_path, &file_info);
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> I think you can get all of this information from the FindInfo that's free when
> you use the enumerator.  Check out DeleteDataModifiedSince above.

hmm.. FindInfo is platform-specific, I think GetFileInfo is better at this
level?

http://codereview.chromium.org/523139/diff/6001/7008
File chrome/browser/in_process_webkit/dom_storage_context.h (right):

http://codereview.chromium.org/523139/diff/6001/7008#newcode29
chrome/browser/in_process_webkit/dom_storage_context.h:29: struct
LocalStorageInfo {
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> This should really be in the same file as the model.

done, and moved the GetLocalStorageInfo over there as well.

http://codereview.chromium.org/523139/diff/6001/7009
File chrome/browser/local_storage_model.cc (right):

http://codereview.chromium.org/523139/diff/6001/7009#newcode16
chrome/browser/local_storage_model.cc:16: void
LocalStorageModel::StartFetching(Callback0::Type* callback) {
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> Use Task* instead.
> 
> Or....I think there's some standard type for a function/method with unbound
> arguments.  It might be possible to do that rather than having the
> GetLocalStorageInfo method?

really good suggestion, thanks! using a Callback1 and passing the vector in it,
removed the GetLocalStorageInfo..
(as above, can't use a Task..)

http://codereview.chromium.org/523139/diff/6001/7009#newcode17
chrome/browser/local_storage_model.cc:17:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> Check the has_finished here..and elsewhere maybe?
> 
> Also verify the completion callback is currently null.

Done.

http://codereview.chromium.org/523139/diff/6001/7009#newcode30
chrome/browser/local_storage_model.cc:30: void
LocalStorageModel::DeleteFile(const FilePath& file_path) {
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> Function order here should generally match that of the .h file.

hmm, not sure I understood this one..

http://codereview.chromium.org/523139/diff/6001/7009#newcode61
chrome/browser/local_storage_model.cc:61:
std::vector<DOMStorageContext::LocalStorageInfo>* local_storage_info_out) {
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> _out seems redundant since this takes a non-const pointer, I think it's
obvious?
>  Or maybe the style guide says you should use out here?
> 
> If you change it, change it in the .h file too.

removed this method, using a Callback1 instead..

http://codereview.chromium.org/523139/diff/6001/7009#newcode73
chrome/browser/local_storage_model.cc:73: if (completion_callback_)
completion_callback_->Run();
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> normally we don't put if statements on one line even if it's possible...it's
> just easier to read

thanks! done.

http://codereview.chromium.org/523139/diff/6001/7004
File chrome/browser/local_storage_model.h (right):

http://codereview.chromium.org/523139/diff/6001/7004#newcode15
chrome/browser/local_storage_model.h:15: // This class fetches local storage
information in the File thread, and notifies
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> WebKit thread

Done.

http://codereview.chromium.org/523139/diff/6001/7004#newcode16
chrome/browser/local_storage_model.h:16: // the UI thread upon completion.
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> I feel like you should go into a bit more detail.

done, let me know if it's clear.

http://codereview.chromium.org/523139/diff/6001/7004#newcode50
chrome/browser/local_storage_model.h:50: DOMStorageContext*
dom_storage_context_;
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> const?
which const? :)

can't be const Foo*, as DeleteFile needs PurgeMemory..
could be Foo* const, but I haven't seen it.. (i.e.,
DOMStorageContext::webkit_context_ could const). I'm happy to change both.

http://codereview.chromium.org/523139/diff/6001/7004#newcode52
chrome/browser/local_storage_model.h:52: Callback0::Type* completion_callback_;
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> Task* ?

changed to Callback1, so that it'll notify with the vector of storage info (and
removed the "GetLocalStorageInfo" method..)
it could be a task, however its parameters are bound, so I'd need to
re-introduce GetLocalStorageInfo..
a callback1 seems simpler, but I'm happy to change!

http://codereview.chromium.org/523139/diff/6001/7001
File chrome/browser/local_storage_model_unittest.cc (right):

http://codereview.chromium.org/523139/diff/6001/7001#newcode35
chrome/browser/local_storage_model_unittest.cc:35: static const
FilePath::CharType* kFilesToCreate[] = {
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> Probably no need for this to be static.

Done.

http://codereview.chromium.org/523139/diff/6001/7001#newcode45
chrome/browser/local_storage_model_unittest.cc:45: FilePath
storage_path(testing_profile_.GetPath());
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> We now have a couple places where we calculate the local storage
> directory....maybe we should add a function to the profile to do it for us?  
> (Is there precedent?) 

agree.. how about adding this function to DOMStorageContext instead, since it
already knows about profile?

http://codereview.chromium.org/523139/diff/6001/7012
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/523139/diff/6001/7012#newcode182
chrome/test/testing_profile.h:182: virtual WebKitContext* GetWebKitContext() {
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> IIRC...Isn't this the same as the default...in which case we should just
delete
> this function?

it's a pure virtual in profile, we need this here..

http://codereview.chromium.org/523139/diff/6001/7012#newcode254
chrome/test/testing_profile.h:254: // WebKitContext, lazily initialized by
GetWebKitContext().
On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> If so, we don't need this.

as above..

[jorlow, 2010-01-11 21:03:47]

http://codereview.chromium.org/523139/diff/6001/7007
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/523139/diff/6001/7007#newcode182
chrome/browser/in_process_webkit/dom_storage_context.cc:182: PurgeMemory();
On 2010/01/11 20:49:01, bulach wrote:
> On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > Create a bug about purging only what's needed, assign it to me, and add a
TODO
> > about it....it's not a big enough deal to do it now tho.
> 
> added a TODO in my name, will get back to it soon.. DeleteDataModifiedSince
> above could also do with a PurgeMemoryForDatabase(foo)..
> on http://crbug.com, I couldn't find any appropriate template, is "defect
report from
> user" a catch-all to use in this case?

Don't use a template..just type it out.

http://codereview.chromium.org/523139/diff/6001/7007#newcode184
chrome/browser/in_process_webkit/dom_storage_context.cc:184: // Rather than just
delete the passed file, let's re-construct its path.
On 2010/01/11 20:49:01, bulach wrote:
> On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > Why?
> 
> hmm. I think I'm being overzealous, but since it's a public method, does it
make
> sense to ensure it'll only ever delete files inside its associated directory?
> file_path could be anything at this point... but anyway, I agree it's probably
> pointless, I'm happy to just deletefile.

Maybe change the function name so it's more clear that you should only be
deleting local storage files with it?

DeleteLocalStorageFile?

http://codereview.chromium.org/523139/diff/6001/7007#newcode217
chrome/browser/in_process_webkit/dom_storage_context.cc:217: file_path++) {
On 2010/01/11 20:49:01, bulach wrote:
> On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > ++blah not blah++
> 
> so dumi said to use blah++ for iterators and ++blah for PODs.. this specific
> code here has been removed, but what's the guideline for future reference?

Did dumi say why to use blah++ for iterators? 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a...
doesn't mention it, but leaves it open...so maybe there's some reason why it's
not as good for iterators?  I don't know of any.

http://codereview.chromium.org/523139/diff/6001/7007#newcode237
chrome/browser/in_process_webkit/dom_storage_context.cc:237: bool ret =
file_util::GetFileInfo(file_path, &file_info);
On 2010/01/11 20:49:01, bulach wrote:
> On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > I think you can get all of this information from the FindInfo that's free
when
> > you use the enumerator.  Check out DeleteDataModifiedSince above.
> 
> hmm.. FindInfo is platform-specific, I think GetFileInfo is better at this
> level?

Make sure you have the proper error handling code for the file going away in
between then.

http://codereview.chromium.org/523139/diff/6001/7009
File chrome/browser/local_storage_model.cc (right):

http://codereview.chromium.org/523139/diff/6001/7009#newcode30
chrome/browser/local_storage_model.cc:30: void
LocalStorageModel::DeleteFile(const FilePath& file_path) {
On 2010/01/11 20:49:01, bulach wrote:
> On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > Function order here should generally match that of the .h file.
> 
> hmm, not sure I understood this one..

You should define functions in the .cc file in the same order they appear in the
.h file generally.

http://codereview.chromium.org/523139/diff/10002/10017
File chrome/browser/browsing_data_local_storage_fetcher.h (right):

http://codereview.chromium.org/523139/diff/10002/10017#newcode5
chrome/browser/browsing_data_local_storage_fetcher.h:5: #ifndef
CHROME_BROWSER_BROWSING_DATA_LOCAL_STORAGE_FETCHER_H_
Hm....on second thought, fetcher might not be the best name since it's also
deleting stuff....on the other hand, I don't really care and can't think of a
better name.

http://codereview.chromium.org/523139/diff/10002/10017#newcode90
chrome/browser/browsing_data_local_storage_fetcher.h:90: };
Use the DISALLOW_blah macro to not allow copying.

http://codereview.chromium.org/523139/diff/10002/10016
File chrome/browser/browsing_data_local_storage_fetcher_unittest.cc (right):

http://codereview.chromium.org/523139/diff/10002/10016#newcode137
chrome/browser/browsing_data_local_storage_fetcher_unittest.cc:137: 
remove line

http://codereview.chromium.org/523139/diff/10002/10008
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/523139/diff/10002/10008#newcode182
chrome/browser/in_process_webkit/dom_storage_context.cc:182: // TODO(bulach):
both this method and DeleteDataModifiedSince could purge
I'd say the DeleteDataModifiedSince case is much less important since the common
case is that everything is erased.

[ian fette, 2010-01-11 22:02:56]

http://codereview.chromium.org/523139/diff/6001/7007
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/523139/diff/6001/7007#newcode217
chrome/browser/in_process_webkit/dom_storage_context.cc:217: file_path++) {
On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> On 2010/01/11 20:49:01, bulach wrote:
> > On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > > ++blah not blah++
> > 
> > so dumi said to use blah++ for iterators and ++blah for PODs.. this specific
> > code here has been removed, but what's the guideline for future reference?
> 
> Did dumi say why to use blah++ for iterators? 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a...
> doesn't mention it, but leaves it open...so maybe there's some reason why it's
> not as good for iterators?  I don't know of any.

Usually, for complex types, pre-increment is a better choice. Namely, because
for post-increment, you normally need to save a temporary copy of the "old"
value around and return that, whereas with pre-increment you can just change the
object and then return it.

Post-increment is often implemented in terms of pre-increment, e.g.

Foo operator++(int) {
  Foo retval(*this);
  ++*this;
  return retval;
}

So, if you are ambivalent, for complex types it's usually better to use ++blah.

[bulach_, 2010-01-11 22:09:04]

thanks, Jeremy!

another batch with the comments addressed, plus a question about the "Fetcher"
part versus "delete-in-another-thread", let me know your thoughts on this.

http://codereview.chromium.org/523139/diff/6001/7007
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/523139/diff/6001/7007#newcode182
chrome/browser/in_process_webkit/dom_storage_context.cc:182: PurgeMemory();
On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> On 2010/01/11 20:49:01, bulach wrote:
> > On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > > Create a bug about purging only what's needed, assign it to me, and add a
> TODO
> > > about it....it's not a big enough deal to do it now tho.
> > 
> > added a TODO in my name, will get back to it soon.. DeleteDataModifiedSince
> > above could also do with a PurgeMemoryForDatabase(foo)..
> > on http://crbug.com, I couldn't find any appropriate template, is "defect
> report from
> > user" a catch-all to use in this case?
> 
> Don't use a template..just type it out.

ahn, got it. thanks!

http://codereview.chromium.org/523139/diff/6001/7007#newcode184
chrome/browser/in_process_webkit/dom_storage_context.cc:184: // Rather than just
delete the passed file, let's re-construct its path.
On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> On 2010/01/11 20:49:01, bulach wrote:
> > On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > > Why?
> > 
> > hmm. I think I'm being overzealous, but since it's a public method, does it
> make
> > sense to ensure it'll only ever delete files inside its associated
directory?
> > file_path could be anything at this point... but anyway, I agree it's
probably
> > pointless, I'm happy to just deletefile.
> 
> Maybe change the function name so it's more clear that you should only be
> deleting local storage files with it?
> 
> DeleteLocalStorageFile?

agree, done.

http://codereview.chromium.org/523139/diff/6001/7007#newcode217
chrome/browser/in_process_webkit/dom_storage_context.cc:217: file_path++) {
On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> On 2010/01/11 20:49:01, bulach wrote:
> > On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > > ++blah not blah++
> > 
> > so dumi said to use blah++ for iterators and ++blah for PODs.. this specific
> > code here has been removed, but what's the guideline for future reference?
> 
> Did dumi say why to use blah++ for iterators? 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a...
> doesn't mention it, but leaves it open...so maybe there's some reason why it's
> not as good for iterators?  I don't know of any.

ahn, thanks for the refresher! the styleguide is clear, "For iterators and other
template types, use pre-increment.". I'll ping dumi...

http://codereview.chromium.org/523139/diff/6001/7007#newcode237
chrome/browser/in_process_webkit/dom_storage_context.cc:237: bool ret =
file_util::GetFileInfo(file_path, &file_info);
On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> On 2010/01/11 20:49:01, bulach wrote:
> > On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > > I think you can get all of this information from the FindInfo that's free
> when
> > > you use the enumerator.  Check out DeleteDataModifiedSince above.
> > 
> > hmm.. FindInfo is platform-specific, I think GetFileInfo is better at this
> > level?
> 
> Make sure you have the proper error handling code for the file going away in
> between then.

good point! done.

http://codereview.chromium.org/523139/diff/6001/7009
File chrome/browser/local_storage_model.cc (right):

http://codereview.chromium.org/523139/diff/6001/7009#newcode30
chrome/browser/local_storage_model.cc:30: void
LocalStorageModel::DeleteFile(const FilePath& file_path) {
On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> On 2010/01/11 20:49:01, bulach wrote:
> > On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > > Function order here should generally match that of the .h file.
> > 
> > hmm, not sure I understood this one..
> 
> You should define functions in the .cc file in the same order they appear in
the
> .h file generally.

I'm probably missing something here, I think they're in the same order?

http://codereview.chromium.org/523139/diff/10002/10017
File chrome/browser/browsing_data_local_storage_fetcher.h (right):

http://codereview.chromium.org/523139/diff/10002/10017#newcode5
chrome/browser/browsing_data_local_storage_fetcher.h:5: #ifndef
CHROME_BROWSER_BROWSING_DATA_LOCAL_STORAGE_FETCHER_H_
On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> Hm....on second thought, fetcher might not be the best name since it's also
> deleting stuff....on the other hand, I don't really care and can't think of a
> better name.

:)

some options:
1. leave as it is..
2. remove "fetcher", possibly add "manager" or "handler"
3. Keep "Fetcher" and extract the "delete-file-in-another-thread" bits in
another utility class. perhaps DBs would also reuse? ..but then again, what do
you suggest for name / location of such utility?

http://codereview.chromium.org/523139/diff/10002/10017#newcode90
chrome/browser/browsing_data_local_storage_fetcher.h:90: };
On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> Use the DISALLOW_blah macro to not allow copying.

Done.

http://codereview.chromium.org/523139/diff/10002/10016
File chrome/browser/browsing_data_local_storage_fetcher_unittest.cc (right):

http://codereview.chromium.org/523139/diff/10002/10016#newcode137
chrome/browser/browsing_data_local_storage_fetcher_unittest.cc:137: 
On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> remove line

Done.

http://codereview.chromium.org/523139/diff/10002/10008
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/523139/diff/10002/10008#newcode182
chrome/browser/in_process_webkit/dom_storage_context.cc:182: // TODO(bulach):
both this method and DeleteDataModifiedSince could purge
On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> I'd say the DeleteDataModifiedSince case is much less important since the
common
> case is that everything is erased.

I fully agree, but since we'll have to implement something that maps a filename
to origin and then purge the associated memory, it'd be nice to use the same
mechanism in ..ModifiedSince()..

[jorlow, 2010-01-11 22:21:49]

Think this was the only question left unresolved...

http://codereview.chromium.org/523139/diff/10002/10017
File chrome/browser/browsing_data_local_storage_fetcher.h (right):

http://codereview.chromium.org/523139/diff/10002/10017#newcode5
chrome/browser/browsing_data_local_storage_fetcher.h:5: #ifndef
CHROME_BROWSER_BROWSING_DATA_LOCAL_STORAGE_FETCHER_H_
On 2010/01/11 22:09:04, bulach wrote:
> On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> > Hm....on second thought, fetcher might not be the best name since it's also
> > deleting stuff....on the other hand, I don't really care and can't think of
a
> > better name.
> 
> :)
> 
> some options:
> 1. leave as it is..
> 2. remove "fetcher", possibly add "manager" or "handler"
> 3. Keep "Fetcher" and extract the "delete-file-in-another-thread" bits in
> another utility class. perhaps DBs would also reuse? ..but then again, what do
> you suggest for name / location of such utility?

We try to avoid "manager" because it doesn't really add any information.  Same
with handler in this context.

Helper doesn't add much information, but there is some precedent for stuff being
blahHelper when they help handle multi-thread stuff.

That's the best I can think of.  Feel free to use it, think of something else,
or leave it as is.

[bulach_, 2010-01-11 23:10:30]

Ian: thanks for the explanation!

Jeremy: used helper instead of fetcher, seems more appropriate.

a final look please?

On 2010/01/11 22:21:49, Jeremy Orlow wrote:
> Think this was the only question left unresolved...
> 
> http://codereview.chromium.org/523139/diff/10002/10017
> File chrome/browser/browsing_data_local_storage_fetcher.h (right):
> 
> http://codereview.chromium.org/523139/diff/10002/10017#newcode5
> chrome/browser/browsing_data_local_storage_fetcher.h:5: #ifndef
> CHROME_BROWSER_BROWSING_DATA_LOCAL_STORAGE_FETCHER_H_
> On 2010/01/11 22:09:04, bulach wrote:
> > On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> > > Hm....on second thought, fetcher might not be the best name since it's
also
> > > deleting stuff....on the other hand, I don't really care and can't think
of
> a
> > > better name.
> > 
> > :)
> > 
> > some options:
> > 1. leave as it is..
> > 2. remove "fetcher", possibly add "manager" or "handler"
> > 3. Keep "Fetcher" and extract the "delete-file-in-another-thread" bits in
> > another utility class. perhaps DBs would also reuse? ..but then again, what
do
> > you suggest for name / location of such utility?
> 
> We try to avoid "manager" because it doesn't really add any information.  Same
> with handler in this context.
> 
> Helper doesn't add much information, but there is some precedent for stuff
being
> blahHelper when they help handle multi-thread stuff.
> 
> That's the best I can think of.  Feel free to use it, think of something else,
> or leave it as is.

[jorlow, 2010-01-12 00:10:08]

Hmm....why didn't these send?

http://codereview.chromium.org/523139/diff/9048/13040
File chrome/browser/browsing_data_local_storage_helper.cc (right):

http://codereview.chromium.org/523139/diff/9048/13040#newcode14
chrome/browser/browsing_data_local_storage_helper.cc:14: #include
"third_party/WebKit/WebKit/chromium/public/WebSecurityOrigin.h"
alphabetical order

http://codereview.chromium.org/523139/diff/9048/13040#newcode21
chrome/browser/browsing_data_local_storage_helper.cc:21: has_finished_(false) {
might want a DCHECK(profile_)

http://codereview.chromium.org/523139/diff/9048/13040#newcode120
chrome/browser/browsing_data_local_storage_helper.cc:120:
DeleteAllLocalStorageFilesInWebKitThread() {
Can the blah::blah go on the same line if void goes above?  If so, that's
preferred.

http://codereview.chromium.org/523139/diff/9048/13038
File chrome/browser/browsing_data_local_storage_helper_unittest.cc (right):

http://codereview.chromium.org/523139/diff/9048/13038#newcode62
chrome/browser/browsing_data_local_storage_helper_unittest.cc:62:
local_storage_info) {
no extra indent

http://codereview.chromium.org/523139/diff/9048/13029
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/523139/diff/9048/13029#newcode213
chrome/browser/cookies_tree_model.cc:213: lower_bound(children().begin(),
This is pretty ugly...can lower_bound( go on the line above and then just indent
the rest by 4 spaces?  even if not, maybe that'd be better than lining up on the
(.

http://codereview.chromium.org/523139/diff/9048/13029#newcode239
chrome/browser/cookies_tree_model.cc:239: return (static_cast<const
CookieTreeLocalStorageNode*>(lhs)->
I'd save these to temps and then compare those->orign or something...this is
pretty hard to read

http://codereview.chromium.org/523139/diff/9048/13036
File chrome/browser/gtk/options/cookies_view.cc (right):

http://codereview.chromium.org/523139/diff/9048/13036#newcode38
chrome/browser/gtk/options/cookies_view.cc:38: void InitDetailStyle(GtkWidget*
entry, GtkStyle* label_style,
BrowserDetail?

http://codereview.chromium.org/523139/diff/9048/13036#newcode370
chrome/browser/gtk/options/cookies_view.cc:370: local_storage_info) {
don't indent further

http://codereview.chromium.org/523139/diff/9048/13036#newcode499
chrome/browser/gtk/options/cookies_view.cc:499: if (table ==
local_storage_details_table_) other = cookie_details_table_;
Usually we do this on 2 lines.

http://codereview.chromium.org/523139/diff/9048/13037
File chrome/browser/gtk/options/cookies_view.h (right):

http://codereview.chromium.org/523139/diff/9048/13037#newcode72
chrome/browser/gtk/options/cookies_view.h:72: local_storage_info);
no extra indent

http://codereview.chromium.org/523139/diff/9048/13031
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/523139/diff/9048/13031#newcode15
chrome/browser/in_process_webkit/dom_storage_context.cc:15: #include
"third_party/WebKit/WebKit/chromium/public/WebString.h"
alpha order

http://codereview.chromium.org/523139/diff/9048/13031#newcode201
chrome/browser/in_process_webkit/dom_storage_context.cc:201: if
(file_path.Extension() == kLocalStorageExtension) {
don't need {}'s

http://codereview.chromium.org/523139/diff/9048/13027
File chrome/browser/views/options/cookies_view.cc (right):

http://codereview.chromium.org/523139/diff/9048/13027#newcode270
chrome/browser/views/options/cookies_view.cc:270: local_storage_info) {
remove 4 spaces

http://codereview.chromium.org/523139/diff/9048/13027#newcode288
chrome/browser/views/options/cookies_view.cc:288: // TODO(bulach): use a proper
label
When is this happening?

Also, what about the folder icons for local storage?  Maybe ask the UI team?

http://codereview.chromium.org/523139/diff/9048/13028
File chrome/browser/views/options/cookies_view.h (right):

http://codereview.chromium.org/523139/diff/9048/13028#newcode32
chrome/browser/views/options/cookies_view.h:32: class
BrowsingDataLocalStorageHelper;
alpha order

http://codereview.chromium.org/523139/diff/9048/13028#newcode200
chrome/browser/views/options/cookies_view.h:200: local_storage_info);
unindent

http://codereview.chromium.org/523139/diff/9048/13028#newcode212
chrome/browser/views/options/cookies_view.h:212: views::View* parent,
nit: I would have put these all on the 2nd line...thus saving a line...not a big
deal tho

http://codereview.chromium.org/523139/diff/9048/13033
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/523139/diff/9048/13033#newcode1094
chrome/chrome_browser.gypi:1094:
'browser/browsing_data_local_storage_helper.cc',
alpha order

http://codereview.chromium.org/523139/diff/9048/13034
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/523139/diff/9048/13034#newcode1110
chrome/chrome_tests.gypi:1110:
'browser/browsing_data_local_storage_helper_unittest.cc',
alpha order

[bulach_, 2010-01-12 00:38:33]

thanks Jeremy! please, keep this style comments coming, I'll try to forget the
old style I used as quickly as I can.. :)

http://codereview.chromium.org/523139/diff/9048/13040
File chrome/browser/browsing_data_local_storage_helper.cc (right):

http://codereview.chromium.org/523139/diff/9048/13040#newcode14
chrome/browser/browsing_data_local_storage_helper.cc:14: #include
"third_party/WebKit/WebKit/chromium/public/WebSecurityOrigin.h"
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> alphabetical order

Done.

http://codereview.chromium.org/523139/diff/9048/13040#newcode21
chrome/browser/browsing_data_local_storage_helper.cc:21: has_finished_(false) {
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> might want a DCHECK(profile_)

Done.

http://codereview.chromium.org/523139/diff/9048/13040#newcode120
chrome/browser/browsing_data_local_storage_helper.cc:120:
DeleteAllLocalStorageFilesInWebKitThread() {
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> Can the blah::blah go on the same line if void goes above?  If so, that's
> preferred.

Done.

http://codereview.chromium.org/523139/diff/9048/13038
File chrome/browser/browsing_data_local_storage_helper_unittest.cc (right):

http://codereview.chromium.org/523139/diff/9048/13038#newcode62
chrome/browser/browsing_data_local_storage_helper_unittest.cc:62:
local_storage_info) {
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> no extra indent

done

http://codereview.chromium.org/523139/diff/9048/13029
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/523139/diff/9048/13029#newcode213
chrome/browser/cookies_tree_model.cc:213: lower_bound(children().begin(),
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> This is pretty ugly...can lower_bound( go on the line above and then just
indent
> the rest by 4 spaces?  even if not, maybe that'd be better than lining up on
the
> (.

Done

http://codereview.chromium.org/523139/diff/9048/13029#newcode239
chrome/browser/cookies_tree_model.cc:239: return (static_cast<const
CookieTreeLocalStorageNode*>(lhs)->
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> I'd save these to temps and then compare those->orign or something...this is
> pretty hard to read

Done.

http://codereview.chromium.org/523139/diff/9048/13036
File chrome/browser/gtk/options/cookies_view.cc (right):

http://codereview.chromium.org/523139/diff/9048/13036#newcode38
chrome/browser/gtk/options/cookies_view.cc:38: void InitDetailStyle(GtkWidget*
entry, GtkStyle* label_style,
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> BrowserDetail?

Done.

http://codereview.chromium.org/523139/diff/9048/13036#newcode370
chrome/browser/gtk/options/cookies_view.cc:370: local_storage_info) {
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> don't indent further

Done.

http://codereview.chromium.org/523139/diff/9048/13036#newcode499
chrome/browser/gtk/options/cookies_view.cc:499: if (table ==
local_storage_details_table_) other = cookie_details_table_;
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> Usually we do this on 2 lines.

Done.

http://codereview.chromium.org/523139/diff/9048/13037
File chrome/browser/gtk/options/cookies_view.h (right):

http://codereview.chromium.org/523139/diff/9048/13037#newcode72
chrome/browser/gtk/options/cookies_view.h:72: local_storage_info);
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> no extra indent

Done.

http://codereview.chromium.org/523139/diff/9048/13031
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/523139/diff/9048/13031#newcode15
chrome/browser/in_process_webkit/dom_storage_context.cc:15: #include
"third_party/WebKit/WebKit/chromium/public/WebString.h"
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> alpha order

Done.

http://codereview.chromium.org/523139/diff/9048/13027
File chrome/browser/views/options/cookies_view.cc (right):

http://codereview.chromium.org/523139/diff/9048/13027#newcode270
chrome/browser/views/options/cookies_view.cc:270: local_storage_info) {
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> remove 4 spaces

Done.

http://codereview.chromium.org/523139/diff/9048/13027#newcode288
chrome/browser/views/options/cookies_view.cc:288: // TODO(bulach): use a proper
label
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> When is this happening?
> 
> Also, what about the folder icons for local storage?  Maybe ask the UI team?

oh, the string is already in, forgot to remove the todo.
is there any list / person I could ask on the UI side?

http://codereview.chromium.org/523139/diff/9048/13028
File chrome/browser/views/options/cookies_view.h (right):

http://codereview.chromium.org/523139/diff/9048/13028#newcode32
chrome/browser/views/options/cookies_view.h:32: class
BrowsingDataLocalStorageHelper;
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> alpha order

Done.

http://codereview.chromium.org/523139/diff/9048/13028#newcode200
chrome/browser/views/options/cookies_view.h:200: local_storage_info);
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> unindent

Done.

http://codereview.chromium.org/523139/diff/9048/13028#newcode212
chrome/browser/views/options/cookies_view.h:212: views::View* parent,
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> nit: I would have put these all on the 2nd line...thus saving a line...not a
big
> deal tho

Done.

http://codereview.chromium.org/523139/diff/9048/13033
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/523139/diff/9048/13033#newcode1094
chrome/chrome_browser.gypi:1094:
'browser/browsing_data_local_storage_helper.cc',
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> alpha order

Done.

http://codereview.chromium.org/523139/diff/9048/13034
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/523139/diff/9048/13034#newcode1110
chrome/chrome_tests.gypi:1110:
'browser/browsing_data_local_storage_helper_unittest.cc',
On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> alpha order

Done.

[jorlow, 2010-01-12 00:40:36]

LGTM from my side....once dumi LGTM's, I'll land.

http://codereview.chromium.org/523139/diff/9048/13027
File chrome/browser/views/options/cookies_view.cc (right):

http://codereview.chromium.org/523139/diff/9048/13027#newcode288
chrome/browser/views/options/cookies_view.cc:288: // TODO(bulach): use a proper
label
On 2010/01/12 00:38:33, bulach wrote:
> On 2010/01/12 00:10:08, Jeremy Orlow wrote:
> > When is this happening?
> > 
> > Also, what about the folder icons for local storage?  Maybe ask the UI team?
> 
> oh, the string is already in, forgot to remove the todo.
> is there any list / person I could ask on the UI side?

I donno...ask the UI team?  brian, glen, and ben.

[dumi, 2010-01-12 01:30:18]

a few more minor comments.

http://codereview.chromium.org/523139/diff/9064/9079
File chrome/browser/browsing_data_local_storage_helper.cc (right):

http://codereview.chromium.org/523139/diff/9064/9079#newcode68
chrome/browser/browsing_data_local_storage_helper.cc:68: DCHECK(!has_finished_);
i'm a bit concerned that you modify the value of this variable on the WebKit
thread, but check its value both on the WebKit and UI threads. it's also not
clear to me what the purpose of this variable is. are you trying to make sure
that FetchLocalStorageInfoInWebKitThread() is called only once? if so, then i
think it's best to not touch has_finished_ in
FetchLocalStorageInfoInWebKitThread() and set its value to true in
NotifyInUIThread(). and maybe rename it to info_fetched_ or
local_storage_info_fetched_ or something like that?

http://codereview.chromium.org/523139/diff/9064/9078
File chrome/browser/browsing_data_local_storage_helper.h (right):

http://codereview.chromium.org/523139/diff/9064/9078#newcode18
chrome/browser/browsing_data_local_storage_helper.h:18: // Clients of this class
need to call StartFetching from the UI thread to
minor: s/Clients/A client/

http://codereview.chromium.org/523139/diff/9064/9068
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/523139/diff/9064/9068#newcode212
chrome/browser/cookies_tree_model.cc:212: std::vector<CookieTreeNode*>::iterator
local_storage_iterator = lower_bound(
minor: for consistency sake, please indent this statement like it's indented in
CookieTreeCookiesNode:AddCookieNode().

http://codereview.chromium.org/523139/diff/9064/9068#newcode225
chrome/browser/cookies_tree_model.cc:225: const CookieTreeCookieNode* left =
good refactoring!

http://codereview.chromium.org/523139/diff/9064/9068#newcode237
chrome/browser/cookies_tree_model.cc:237: return (static_cast<const
CookieTreeLocalStorageNode*>(lhs)->
please refactor to look like the method above this one.

http://codereview.chromium.org/523139/diff/9064/9069
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/523139/diff/9064/9069#newcode286
chrome/browser/cookies_tree_model.h:286:
scoped_refptr<BrowsingDataLocalStorageHelper> local_storage_model_;
minor: i'd format this as:

...
CookieList all_cookies_;

scoped_refptr<> local_storage_model_;
LocalStorageInfoList local_storage_info_list_;

this way the local storage members are grouped up together.

http://codereview.chromium.org/523139/diff/9064/9075
File chrome/browser/gtk/options/cookies_view.cc (right):

http://codereview.chromium.org/523139/diff/9064/9075#newcode197
chrome/browser/gtk/options/cookies_view.cc:197:
gtk_frame_set_shadow_type(GTK_FRAME(cookie_details_frame),
GTK_SHADOW_ETCHED_IN);
line > 80 chars.

http://codereview.chromium.org/523139/diff/9064/9075#newcode255
chrome/browser/gtk/options/cookies_view.cc:255: // Cookie details.
please leave an empty line before this one as in the original code.

http://codereview.chromium.org/523139/diff/9064/9075#newcode263
chrome/browser/gtk/options/cookies_view.cc:263: // Local storage details.
please leave an empty line before this one.

http://codereview.chromium.org/523139/diff/9064/9074
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/523139/diff/9064/9074#newcode183
chrome/test/testing_profile.h:183: if (webkit_context_ == NULL) {
no need for {}, and i believe we prefer 'if (!blah)' to 'if (blah == NULL)'.

[dumi, 2010-01-12 01:36:04]

On 2010/01/11 22:02:56, ian fette wrote:
> http://codereview.chromium.org/523139/diff/6001/7007
> File chrome/browser/in_process_webkit/dom_storage_context.cc (right):
> 
> http://codereview.chromium.org/523139/diff/6001/7007#newcode217
> chrome/browser/in_process_webkit/dom_storage_context.cc:217: file_path++) {
> On 2010/01/11 21:03:48, Jeremy Orlow wrote:
> > On 2010/01/11 20:49:01, bulach wrote:
> > > On 2010/01/09 04:20:49, Jeremy Orlow wrote:
> > > > ++blah not blah++
> > > 
> > > so dumi said to use blah++ for iterators and ++blah for PODs.. this
specific
> > > code here has been removed, but what's the guideline for future reference?
> > 
> > Did dumi say why to use blah++ for iterators? 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a...
> > doesn't mention it, but leaves it open...so maybe there's some reason why
it's
> > not as good for iterators?  I don't know of any.
> 
> Usually, for complex types, pre-increment is a better choice. Namely, because
> for post-increment, you normally need to save a temporary copy of the "old"
> value around and return that, whereas with pre-increment you can just change
the
> object and then return it.
> 
> Post-increment is often implemented in terms of pre-increment, e.g.
> 
> Foo operator++(int) {
>   Foo retval(*this);
>   ++*this;
>   return retval;
> }
> 
> So, if you are ambivalent, for complex types it's usually better to use
++blah.

oops, i stand corrected. sorry for the confusion, marcus. i always used
post-increment for iterators and was never asked to change it to pre-increment,
so i assumed that was the preferred style.

[jorlow, 2010-01-12 01:52:55]

http://codereview.chromium.org/523139/diff/9064/9079
File chrome/browser/browsing_data_local_storage_helper.cc (right):

http://codereview.chromium.org/523139/diff/9064/9079#newcode68
chrome/browser/browsing_data_local_storage_helper.cc:68: DCHECK(!has_finished_);
On 2010/01/12 01:30:18, dumi wrote:
> i'm a bit concerned that you modify the value of this variable on the WebKit
> thread, but check its value both on the WebKit and UI threads. it's also not
> clear to me what the purpose of this variable is. are you trying to make sure
> that FetchLocalStorageInfoInWebKitThread() is called only once? if so, then i
> think it's best to not touch has_finished_ in
> FetchLocalStorageInfoInWebKitThread() and set its value to true in
> NotifyInUIThread(). and maybe rename it to info_fetched_ or
> local_storage_info_fetched_ or something like that?

I think it's just a sanity check.  If so, there's precedent for not worrying
about these not being thread safe...a shutdown_ variable is used in a lot of
classes, for example.  (The check can produce false negatives but never
positives.)

I'm not against adding more variables so the state machine is even more explicit
tho.

[bulach_, 2010-01-12 21:45:10]

thanks both! another look please?

as for the ++iterator++: since I'm still learning the style around here, I'll
take whatever you all say, however conflicting it may be :), so thanks again
both for the clarification.

http://codereview.chromium.org/523139/diff/9064/9079
File chrome/browser/browsing_data_local_storage_helper.cc (right):

http://codereview.chromium.org/523139/diff/9064/9079#newcode68
chrome/browser/browsing_data_local_storage_helper.cc:68: DCHECK(!has_finished_);
On 2010/01/12 01:52:55, Jeremy Orlow wrote:
> On 2010/01/12 01:30:18, dumi wrote:
> > i'm a bit concerned that you modify the value of this variable on the WebKit
> > thread, but check its value both on the WebKit and UI threads. it's also not
> > clear to me what the purpose of this variable is. are you trying to make
sure
> > that FetchLocalStorageInfoInWebKitThread() is called only once? if so, then
i
> > think it's best to not touch has_finished_ in
> > FetchLocalStorageInfoInWebKitThread() and set its value to true in
> > NotifyInUIThread(). and maybe rename it to info_fetched_ or
> > local_storage_info_fetched_ or something like that?
> 
> I think it's just a sanity check.  If so, there's precedent for not worrying
> about these not being thread safe...a shutdown_ variable is used in a lot of
> classes, for example.  (The check can produce false negatives but never
> positives.)
> 
> I'm not against adding more variables so the state machine is even more
explicit
> tho.

very good points, thanks! I added as a sanity check, to prevent the WEBKIT
thread to populate the vector whilst the callback is in progress in the UI
thread..
I changed to "is_fetching", and made it safer, i.e., only ever test / modify in
the UI thread, so it prevents calling "StartFetching" again before the callback
is notified.

http://codereview.chromium.org/523139/diff/9064/9078
File chrome/browser/browsing_data_local_storage_helper.h (right):

http://codereview.chromium.org/523139/diff/9064/9078#newcode18
chrome/browser/browsing_data_local_storage_helper.h:18: // Clients of this class
need to call StartFetching from the UI thread to
On 2010/01/12 01:30:18, dumi wrote:
> minor: s/Clients/A client/

Done.

http://codereview.chromium.org/523139/diff/9064/9068
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/523139/diff/9064/9068#newcode212
chrome/browser/cookies_tree_model.cc:212: std::vector<CookieTreeNode*>::iterator
local_storage_iterator = lower_bound(
On 2010/01/12 01:30:18, dumi wrote:
> minor: for consistency sake, please indent this statement like it's indented
in
> CookieTreeCookiesNode:AddCookieNode().

Done.

http://codereview.chromium.org/523139/diff/9064/9068#newcode225
chrome/browser/cookies_tree_model.cc:225: const CookieTreeCookieNode* left =
On 2010/01/12 01:30:18, dumi wrote:
> good refactoring!

thanks! :)

http://codereview.chromium.org/523139/diff/9064/9068#newcode237
chrome/browser/cookies_tree_model.cc:237: return (static_cast<const
CookieTreeLocalStorageNode*>(lhs)->
On 2010/01/12 01:30:18, dumi wrote:
> please refactor to look like the method above this one.

Done.

http://codereview.chromium.org/523139/diff/9064/9069
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/523139/diff/9064/9069#newcode286
chrome/browser/cookies_tree_model.h:286:
scoped_refptr<BrowsingDataLocalStorageHelper> local_storage_model_;
On 2010/01/12 01:30:18, dumi wrote:
> minor: i'd format this as:
> 
> ...
> CookieList all_cookies_;
> 
> scoped_refptr<> local_storage_model_;
> LocalStorageInfoList local_storage_info_list_;
> 
> this way the local storage members are grouped up together.

Done.

http://codereview.chromium.org/523139/diff/9064/9075
File chrome/browser/gtk/options/cookies_view.cc (right):

http://codereview.chromium.org/523139/diff/9064/9075#newcode197
chrome/browser/gtk/options/cookies_view.cc:197:
gtk_frame_set_shadow_type(GTK_FRAME(cookie_details_frame),
GTK_SHADOW_ETCHED_IN);
On 2010/01/12 01:30:18, dumi wrote:
> line > 80 chars.

Done.

http://codereview.chromium.org/523139/diff/9064/9075#newcode255
chrome/browser/gtk/options/cookies_view.cc:255: // Cookie details.
On 2010/01/12 01:30:18, dumi wrote:
> please leave an empty line before this one as in the original code.

Done.

http://codereview.chromium.org/523139/diff/9064/9075#newcode263
chrome/browser/gtk/options/cookies_view.cc:263: // Local storage details.
On 2010/01/12 01:30:18, dumi wrote:
> please leave an empty line before this one.

Done.

http://codereview.chromium.org/523139/diff/9064/9074
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/523139/diff/9064/9074#newcode183
chrome/test/testing_profile.h:183: if (webkit_context_ == NULL) {
On 2010/01/12 01:30:18, dumi wrote:
> no need for {}, and i believe we prefer 'if (!blah)' to 'if (blah == NULL)'.

I promise I'll learn! :)

[jorlow, 2010-01-12 21:57:10]

I'll look before landing....I assume it's all good.

On Tue, Jan 12, 2010 at 1:45 PM, <bulach@google.com> wrote:

> thanks both! another look please?
>
> as for the ++iterator++: since I'm still learning the style around here,
> I'll
> take whatever you all say, however conflicting it may be :), so thanks
> again
> both for the clarification.
>
>
>
> http://codereview.chromium.org/523139/diff/9064/9079
> File chrome/browser/browsing_data_local_storage_helper.cc (right):
>
> http://codereview.chromium.org/523139/diff/9064/9079#newcode68
> chrome/browser/browsing_data_local_storage_helper.cc:68:
> DCHECK(!has_finished_);
> On 2010/01/12 01:52:55, Jeremy Orlow wrote:
>
>> On 2010/01/12 01:30:18, dumi wrote:
>> > i'm a bit concerned that you modify the value of this variable on
>>
> the WebKit
>
>> > thread, but check its value both on the WebKit and UI threads. it's
>>
> also not
>
>> > clear to me what the purpose of this variable is. are you trying to
>>
> make sure
>
>> > that FetchLocalStorageInfoInWebKitThread() is called only once? if
>>
> so, then i
>
>> > think it's best to not touch has_finished_ in
>> > FetchLocalStorageInfoInWebKitThread() and set its value to true in
>> > NotifyInUIThread(). and maybe rename it to info_fetched_ or
>> > local_storage_info_fetched_ or something like that?
>>
>
>  I think it's just a sanity check.  If so, there's precedent for not
>>
> worrying
>
>> about these not being thread safe...a shutdown_ variable is used in a
>>
> lot of
>
>> classes, for example.  (The check can produce false negatives but
>>
> never
>
>> positives.)
>>
>
>  I'm not against adding more variables so the state machine is even
>>
> more explicit
>
>> tho.
>>
>
> very good points, thanks! I added as a sanity check, to prevent the
> WEBKIT thread to populate the vector whilst the callback is in progress
> in the UI thread..
> I changed to "is_fetching", and made it safer, i.e., only ever test /
> modify in the UI thread, so it prevents calling "StartFetching" again
> before the callback is notified.
>
>
> http://codereview.chromium.org/523139/diff/9064/9078
> File chrome/browser/browsing_data_local_storage_helper.h (right):
>
> http://codereview.chromium.org/523139/diff/9064/9078#newcode18
> chrome/browser/browsing_data_local_storage_helper.h:18: // Clients of
> this class need to call StartFetching from the UI thread to
> On 2010/01/12 01:30:18, dumi wrote:
>
>> minor: s/Clients/A client/
>>
>
> Done.
>
>
> http://codereview.chromium.org/523139/diff/9064/9068
> File chrome/browser/cookies_tree_model.cc (right):
>
> http://codereview.chromium.org/523139/diff/9064/9068#newcode212
> chrome/browser/cookies_tree_model.cc:212:
> std::vector<CookieTreeNode*>::iterator local_storage_iterator =
> lower_bound(
> On 2010/01/12 01:30:18, dumi wrote:
>
>> minor: for consistency sake, please indent this statement like it's
>>
> indented in
>
>> CookieTreeCookiesNode:AddCookieNode().
>>
>
> Done.
>
>
> http://codereview.chromium.org/523139/diff/9064/9068#newcode225
> chrome/browser/cookies_tree_model.cc:225: const CookieTreeCookieNode*
> left =
> On 2010/01/12 01:30:18, dumi wrote:
>
>> good refactoring!
>>
>
> thanks! :)
>
>
> http://codereview.chromium.org/523139/diff/9064/9068#newcode237
> chrome/browser/cookies_tree_model.cc:237: return (static_cast<const
> CookieTreeLocalStorageNode*>(lhs)->
> On 2010/01/12 01:30:18, dumi wrote:
>
>> please refactor to look like the method above this one.
>>
>
> Done.
>
>
> http://codereview.chromium.org/523139/diff/9064/9069
> File chrome/browser/cookies_tree_model.h (right):
>
> http://codereview.chromium.org/523139/diff/9064/9069#newcode286
> chrome/browser/cookies_tree_model.h:286:
> scoped_refptr<BrowsingDataLocalStorageHelper> local_storage_model_;
> On 2010/01/12 01:30:18, dumi wrote:
>
>> minor: i'd format this as:
>>
>
>  ...
>> CookieList all_cookies_;
>>
>
>  scoped_refptr<> local_storage_model_;
>> LocalStorageInfoList local_storage_info_list_;
>>
>
>  this way the local storage members are grouped up together.
>>
>
> Done.
>
>
> http://codereview.chromium.org/523139/diff/9064/9075
> File chrome/browser/gtk/options/cookies_view.cc (right):
>
> http://codereview.chromium.org/523139/diff/9064/9075#newcode197
> chrome/browser/gtk/options/cookies_view.cc:197:
> gtk_frame_set_shadow_type(GTK_FRAME(cookie_details_frame),
> GTK_SHADOW_ETCHED_IN);
> On 2010/01/12 01:30:18, dumi wrote:
>
>> line > 80 chars.
>>
>
> Done.
>
>
> http://codereview.chromium.org/523139/diff/9064/9075#newcode255
> chrome/browser/gtk/options/cookies_view.cc:255: // Cookie details.
> On 2010/01/12 01:30:18, dumi wrote:
>
>> please leave an empty line before this one as in the original code.
>>
>
> Done.
>
>
> http://codereview.chromium.org/523139/diff/9064/9075#newcode263
> chrome/browser/gtk/options/cookies_view.cc:263: // Local storage
> details.
> On 2010/01/12 01:30:18, dumi wrote:
>
>> please leave an empty line before this one.
>>
>
> Done.
>
>
> http://codereview.chromium.org/523139/diff/9064/9074
> File chrome/test/testing_profile.h (right):
>
> http://codereview.chromium.org/523139/diff/9064/9074#newcode183
> chrome/test/testing_profile.h:183: if (webkit_context_ == NULL) {
> On 2010/01/12 01:30:18, dumi wrote:
>
>> no need for {}, and i believe we prefer 'if (!blah)' to 'if (blah ==
>>
> NULL)'.
>
> I promise I'll learn! :)
>
>
> http://codereview.chromium.org/523139
>

[bulach_, 2010-01-12 21:59:17]

Yes, apart from the icon for the local storage node.. who should I ask?

On Tue, Jan 12, 2010 at 1:56 PM, Jeremy Orlow <jorlow@chromium.org> wrote:
> I'll look before landing....I assume it's all good.
>
> On Tue, Jan 12, 2010 at 1:45 PM, <bulach@google.com> wrote:
>>
>> thanks both! another look please?
>>
>> as for the ++iterator++: since I'm still learning the style around here,
>> I'll
>> take whatever you all say, however conflicting it may be :), so thanks
>> again
>> both for the clarification.
>>
>>
>> http://codereview.chromium.org/523139/diff/9064/9079
>> File chrome/browser/browsing_data_local_storage_helper.cc (right):
>>
>> http://codereview.chromium.org/523139/diff/9064/9079#newcode68
>> chrome/browser/browsing_data_local_storage_helper.cc:68:
>> DCHECK(!has_finished_);
>> On 2010/01/12 01:52:55, Jeremy Orlow wrote:
>>>
>>> On 2010/01/12 01:30:18, dumi wrote:
>>> > i'm a bit concerned that you modify the value of this variable on
>>
>> the WebKit
>>>
>>> > thread, but check its value both on the WebKit and UI threads. it's
>>
>> also not
>>>
>>> > clear to me what the purpose of this variable is. are you trying to
>>
>> make sure
>>>
>>> > that FetchLocalStorageInfoInWebKitThread() is called only once? if
>>
>> so, then i
>>>
>>> > think it's best to not touch has_finished_ in
>>> > FetchLocalStorageInfoInWebKitThread() and set its value to true in
>>> > NotifyInUIThread(). and maybe rename it to info_fetched_ or
>>> > local_storage_info_fetched_ or something like that?
>>
>>> I think it's just a sanity check.  If so, there's precedent for not
>>
>> worrying
>>>
>>> about these not being thread safe...a shutdown_ variable is used in a
>>
>> lot of
>>>
>>> classes, for example.  (The check can produce false negatives but
>>
>> never
>>>
>>> positives.)
>>
>>> I'm not against adding more variables so the state machine is even
>>
>> more explicit
>>>
>>> tho.
>>
>> very good points, thanks! I added as a sanity check, to prevent the
>> WEBKIT thread to populate the vector whilst the callback is in progress
>> in the UI thread..
>> I changed to "is_fetching", and made it safer, i.e., only ever test /
>> modify in the UI thread, so it prevents calling "StartFetching" again
>> before the callback is notified.
>>
>> http://codereview.chromium.org/523139/diff/9064/9078
>> File chrome/browser/browsing_data_local_storage_helper.h (right):
>>
>> http://codereview.chromium.org/523139/diff/9064/9078#newcode18
>> chrome/browser/browsing_data_local_storage_helper.h:18: // Clients of
>> this class need to call StartFetching from the UI thread to
>> On 2010/01/12 01:30:18, dumi wrote:
>>>
>>> minor: s/Clients/A client/
>>
>> Done.
>>
>> http://codereview.chromium.org/523139/diff/9064/9068
>> File chrome/browser/cookies_tree_model.cc (right):
>>
>> http://codereview.chromium.org/523139/diff/9064/9068#newcode212
>> chrome/browser/cookies_tree_model.cc:212:
>> std::vector<CookieTreeNode*>::iterator local_storage_iterator =
>> lower_bound(
>> On 2010/01/12 01:30:18, dumi wrote:
>>>
>>> minor: for consistency sake, please indent this statement like it's
>>
>> indented in
>>>
>>> CookieTreeCookiesNode:AddCookieNode().
>>
>> Done.
>>
>> http://codereview.chromium.org/523139/diff/9064/9068#newcode225
>> chrome/browser/cookies_tree_model.cc:225: const CookieTreeCookieNode*
>> left =
>> On 2010/01/12 01:30:18, dumi wrote:
>>>
>>> good refactoring!
>>
>> thanks! :)
>>
>> http://codereview.chromium.org/523139/diff/9064/9068#newcode237
>> chrome/browser/cookies_tree_model.cc:237: return (static_cast<const
>> CookieTreeLocalStorageNode*>(lhs)->
>> On 2010/01/12 01:30:18, dumi wrote:
>>>
>>> please refactor to look like the method above this one.
>>
>> Done.
>>
>> http://codereview.chromium.org/523139/diff/9064/9069
>> File chrome/browser/cookies_tree_model.h (right):
>>
>> http://codereview.chromium.org/523139/diff/9064/9069#newcode286
>> chrome/browser/cookies_tree_model.h:286:
>> scoped_refptr<BrowsingDataLocalStorageHelper> local_storage_model_;
>> On 2010/01/12 01:30:18, dumi wrote:
>>>
>>> minor: i'd format this as:
>>
>>> ...
>>> CookieList all_cookies_;
>>
>>> scoped_refptr<> local_storage_model_;
>>> LocalStorageInfoList local_storage_info_list_;
>>
>>> this way the local storage members are grouped up together.
>>
>> Done.
>>
>> http://codereview.chromium.org/523139/diff/9064/9075
>> File chrome/browser/gtk/options/cookies_view.cc (right):
>>
>> http://codereview.chromium.org/523139/diff/9064/9075#newcode197
>> chrome/browser/gtk/options/cookies_view.cc:197:
>> gtk_frame_set_shadow_type(GTK_FRAME(cookie_details_frame),
>> GTK_SHADOW_ETCHED_IN);
>> On 2010/01/12 01:30:18, dumi wrote:
>>>
>>> line > 80 chars.
>>
>> Done.
>>
>> http://codereview.chromium.org/523139/diff/9064/9075#newcode255
>> chrome/browser/gtk/options/cookies_view.cc:255: // Cookie details.
>> On 2010/01/12 01:30:18, dumi wrote:
>>>
>>> please leave an empty line before this one as in the original code.
>>
>> Done.
>>
>> http://codereview.chromium.org/523139/diff/9064/9075#newcode263
>> chrome/browser/gtk/options/cookies_view.cc:263: // Local storage
>> details.
>> On 2010/01/12 01:30:18, dumi wrote:
>>>
>>> please leave an empty line before this one.
>>
>> Done.
>>
>> http://codereview.chromium.org/523139/diff/9064/9074
>> File chrome/test/testing_profile.h (right):
>>
>> http://codereview.chromium.org/523139/diff/9064/9074#newcode183
>> chrome/test/testing_profile.h:183: if (webkit_context_ == NULL) {
>> On 2010/01/12 01:30:18, dumi wrote:
>>>
>>> no need for {}, and i believe we prefer 'if (!blah)' to 'if (blah ==
>>
>> NULL)'.
>>
>> I promise I'll learn! :)
>>
>> http://codereview.chromium.org/523139
>
>

[jorlow, 2010-01-12 22:03:50]

I'll land it once dumi LGTMs.

Email glen, ben, and brian about the icon.

On Tue, Jan 12, 2010 at 1:59 PM, Marcus Bulach <bulach@google.com> wrote:

> Yes, apart from the icon for the local storage node.. who should I ask?
>
> On Tue, Jan 12, 2010 at 1:56 PM, Jeremy Orlow <jorlow@chromium.org> wrote:
> > I'll look before landing....I assume it's all good.
> >
> > On Tue, Jan 12, 2010 at 1:45 PM, <bulach@google.com> wrote:
> >>
> >> thanks both! another look please?
> >>
> >> as for the ++iterator++: since I'm still learning the style around here,
> >> I'll
> >> take whatever you all say, however conflicting it may be :), so thanks
> >> again
> >> both for the clarification.
> >>
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9079
> >> File chrome/browser/browsing_data_local_storage_helper.cc (right):
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9079#newcode68
> >> chrome/browser/browsing_data_local_storage_helper.cc:68:
> >> DCHECK(!has_finished_);
> >> On 2010/01/12 01:52:55, Jeremy Orlow wrote:
> >>>
> >>> On 2010/01/12 01:30:18, dumi wrote:
> >>> > i'm a bit concerned that you modify the value of this variable on
> >>
> >> the WebKit
> >>>
> >>> > thread, but check its value both on the WebKit and UI threads. it's
> >>
> >> also not
> >>>
> >>> > clear to me what the purpose of this variable is. are you trying to
> >>
> >> make sure
> >>>
> >>> > that FetchLocalStorageInfoInWebKitThread() is called only once? if
> >>
> >> so, then i
> >>>
> >>> > think it's best to not touch has_finished_ in
> >>> > FetchLocalStorageInfoInWebKitThread() and set its value to true in
> >>> > NotifyInUIThread(). and maybe rename it to info_fetched_ or
> >>> > local_storage_info_fetched_ or something like that?
> >>
> >>> I think it's just a sanity check.  If so, there's precedent for not
> >>
> >> worrying
> >>>
> >>> about these not being thread safe...a shutdown_ variable is used in a
> >>
> >> lot of
> >>>
> >>> classes, for example.  (The check can produce false negatives but
> >>
> >> never
> >>>
> >>> positives.)
> >>
> >>> I'm not against adding more variables so the state machine is even
> >>
> >> more explicit
> >>>
> >>> tho.
> >>
> >> very good points, thanks! I added as a sanity check, to prevent the
> >> WEBKIT thread to populate the vector whilst the callback is in progress
> >> in the UI thread..
> >> I changed to "is_fetching", and made it safer, i.e., only ever test /
> >> modify in the UI thread, so it prevents calling "StartFetching" again
> >> before the callback is notified.
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9078
> >> File chrome/browser/browsing_data_local_storage_helper.h (right):
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9078#newcode18
> >> chrome/browser/browsing_data_local_storage_helper.h:18: // Clients of
> >> this class need to call StartFetching from the UI thread to
> >> On 2010/01/12 01:30:18, dumi wrote:
> >>>
> >>> minor: s/Clients/A client/
> >>
> >> Done.
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9068
> >> File chrome/browser/cookies_tree_model.cc (right):
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9068#newcode212
> >> chrome/browser/cookies_tree_model.cc:212:
> >> std::vector<CookieTreeNode*>::iterator local_storage_iterator =
> >> lower_bound(
> >> On 2010/01/12 01:30:18, dumi wrote:
> >>>
> >>> minor: for consistency sake, please indent this statement like it's
> >>
> >> indented in
> >>>
> >>> CookieTreeCookiesNode:AddCookieNode().
> >>
> >> Done.
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9068#newcode225
> >> chrome/browser/cookies_tree_model.cc:225: const CookieTreeCookieNode*
> >> left =
> >> On 2010/01/12 01:30:18, dumi wrote:
> >>>
> >>> good refactoring!
> >>
> >> thanks! :)
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9068#newcode237
> >> chrome/browser/cookies_tree_model.cc:237: return (static_cast<const
> >> CookieTreeLocalStorageNode*>(lhs)->
> >> On 2010/01/12 01:30:18, dumi wrote:
> >>>
> >>> please refactor to look like the method above this one.
> >>
> >> Done.
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9069
> >> File chrome/browser/cookies_tree_model.h (right):
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9069#newcode286
> >> chrome/browser/cookies_tree_model.h:286:
> >> scoped_refptr<BrowsingDataLocalStorageHelper> local_storage_model_;
> >> On 2010/01/12 01:30:18, dumi wrote:
> >>>
> >>> minor: i'd format this as:
> >>
> >>> ...
> >>> CookieList all_cookies_;
> >>
> >>> scoped_refptr<> local_storage_model_;
> >>> LocalStorageInfoList local_storage_info_list_;
> >>
> >>> this way the local storage members are grouped up together.
> >>
> >> Done.
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9075
> >> File chrome/browser/gtk/options/cookies_view.cc (right):
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9075#newcode197
> >> chrome/browser/gtk/options/cookies_view.cc:197:
> >> gtk_frame_set_shadow_type(GTK_FRAME(cookie_details_frame),
> >> GTK_SHADOW_ETCHED_IN);
> >> On 2010/01/12 01:30:18, dumi wrote:
> >>>
> >>> line > 80 chars.
> >>
> >> Done.
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9075#newcode255
> >> chrome/browser/gtk/options/cookies_view.cc:255: // Cookie details.
> >> On 2010/01/12 01:30:18, dumi wrote:
> >>>
> >>> please leave an empty line before this one as in the original code.
> >>
> >> Done.
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9075#newcode263
> >> chrome/browser/gtk/options/cookies_view.cc:263: // Local storage
> >> details.
> >> On 2010/01/12 01:30:18, dumi wrote:
> >>>
> >>> please leave an empty line before this one.
> >>
> >> Done.
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9074
> >> File chrome/test/testing_profile.h (right):
> >>
> >> http://codereview.chromium.org/523139/diff/9064/9074#newcode183
> >> chrome/test/testing_profile.h:183: if (webkit_context_ == NULL) {
> >> On 2010/01/12 01:30:18, dumi wrote:
> >>>
> >>> no need for {}, and i believe we prefer 'if (!blah)' to 'if (blah ==
> >>
> >> NULL)'.
> >>
> >> I promise I'll learn! :)
> >>
> >> http://codereview.chromium.org/523139
> >
> >
>

[dumi, 2010-01-12 23:45:44]

LGTM

http://codereview.chromium.org/523139/diff/7028/6024
File chrome/browser/browsing_data_local_storage_helper.cc (right):

http://codereview.chromium.org/523139/diff/7028/6024#newcode69
chrome/browser/browsing_data_local_storage_helper.cc:69: DCHECK(is_fetching_);
you're still checking is_fetching_ on the WebKit thread. like jeremy said, it's
probably no big deal. just wanted to make sure you meant to leave this here
since your comment said it should only be checked/modified on the UI thread.

http://codereview.chromium.org/523139/diff/7028/6014
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/523139/diff/7028/6014#newcode278
chrome/browser/cookies_tree_model.h:278: const
std::vector<BrowsingDataLocalStorageHelper::LocalStorageInfo>&
use the LocalStorageInfoList typedef you added.

[jorlow, 2010-01-13 23:45:46]

Marcus, can you please respond to dumi's comments, sync, and upload?  I'll land
then.

[bulach_, 2010-01-16 02:08:17]

thanks Dumi and Jeremy!

synced, resolved, addressed all the remaining comments...

please, take another look and let's land this on Monday so that I'll be around
to watch the bots..

http://codereview.chromium.org/523139/diff/7028/6024
File chrome/browser/browsing_data_local_storage_helper.cc (right):

http://codereview.chromium.org/523139/diff/7028/6024#newcode69
chrome/browser/browsing_data_local_storage_helper.cc:69: DCHECK(is_fetching_);
On 2010/01/12 23:45:44, dumi wrote:
> you're still checking is_fetching_ on the WebKit thread. like jeremy said,
it's
> probably no big deal. just wanted to make sure you meant to leave this here
> since your comment said it should only be checked/modified on the UI thread.

ops, good catch, thanks!I removed the check, the important checks are in
StartFetching / NotifyInUiThread, both in UI thread.

http://codereview.chromium.org/523139/diff/7028/6014
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/523139/diff/7028/6014#newcode278
chrome/browser/cookies_tree_model.h:278: const
std::vector<BrowsingDataLocalStorageHelper::LocalStorageInfo>&
On 2010/01/12 23:45:44, dumi wrote:
> use the LocalStorageInfoList typedef you added.

Done.

[jorlow, 2010-01-16 02:10:27]

We're not here Monday.  How about tues?

On Fri, Jan 15, 2010 at 6:08 PM, <bulach@google.com> wrote:

> thanks Dumi and Jeremy!
>
> synced, resolved, addressed all the remaining comments...
>
> please, take another look and let's land this on Monday so that I'll be
> around
> to watch the bots..
>
>
>
> http://codereview.chromium.org/523139/diff/7028/6024
> File chrome/browser/browsing_data_local_storage_helper.cc (right):
>
> http://codereview.chromium.org/523139/diff/7028/6024#newcode69
> chrome/browser/browsing_data_local_storage_helper.cc:69:
> DCHECK(is_fetching_);
> On 2010/01/12 23:45:44, dumi wrote:
>
>> you're still checking is_fetching_ on the WebKit thread. like jeremy
>>
> said, it's
>
>> probably no big deal. just wanted to make sure you meant to leave this
>>
> here
>
>> since your comment said it should only be checked/modified on the UI
>>
> thread.
>
> ops, good catch, thanks!I removed the check, the important checks are in
> StartFetching / NotifyInUiThread, both in UI thread.
>
>
> http://codereview.chromium.org/523139/diff/7028/6014
> File chrome/browser/cookies_tree_model.h (right):
>
> http://codereview.chromium.org/523139/diff/7028/6014#newcode278
> chrome/browser/cookies_tree_model.h:278: const
> std::vector<BrowsingDataLocalStorageHelper::LocalStorageInfo>&
> On 2010/01/12 23:45:44, dumi wrote:
>
>> use the LocalStorageInfoList typedef you added.
>>
>
> Done.
>
>
> http://codereview.chromium.org/523139
>

[bulach_, 2010-01-16 02:11:37]

Yep, sorry: "logical" monday, not physical.. ;)

On Fri, Jan 15, 2010 at 6:09 PM, Jeremy Orlow <jorlow@chromium.org> wrote:
> We're not here Monday.  How about tues?
>
> On Fri, Jan 15, 2010 at 6:08 PM, <bulach@google.com> wrote:
>>
>> thanks Dumi and Jeremy!
>>
>> synced, resolved, addressed all the remaining comments...
>>
>> please, take another look and let's land this on Monday so that I'll be
>> around
>> to watch the bots..
>>
>>
>> http://codereview.chromium.org/523139/diff/7028/6024
>> File chrome/browser/browsing_data_local_storage_helper.cc (right):
>>
>> http://codereview.chromium.org/523139/diff/7028/6024#newcode69
>> chrome/browser/browsing_data_local_storage_helper.cc:69:
>> DCHECK(is_fetching_);
>> On 2010/01/12 23:45:44, dumi wrote:
>>>
>>> you're still checking is_fetching_ on the WebKit thread. like jeremy
>>
>> said, it's
>>>
>>> probably no big deal. just wanted to make sure you meant to leave this
>>
>> here
>>>
>>> since your comment said it should only be checked/modified on the UI
>>
>> thread.
>>
>> ops, good catch, thanks!I removed the check, the important checks are in
>> StartFetching / NotifyInUiThread, both in UI thread.
>>
>> http://codereview.chromium.org/523139/diff/7028/6014
>> File chrome/browser/cookies_tree_model.h (right):
>>
>> http://codereview.chromium.org/523139/diff/7028/6014#newcode278
>> chrome/browser/cookies_tree_model.h:278: const
>> std::vector<BrowsingDataLocalStorageHelper::LocalStorageInfo>&
>> On 2010/01/12 23:45:44, dumi wrote:
>>>
>>> use the LocalStorageInfoList typedef you added.
>>
>> Done.
>>
>> http://codereview.chromium.org/523139
>
>

[bulach_, 2010-01-20 22:22:47]

hi jeremy,

first things first: many thanks for helping with reverting this patch, and of
course, apologies for the troubles: I completely missed two important steps: try
server, and unit tests (I added "browser tests" and didn't spot "unit test",
ouch...!)
:( that will teach me...

anyways: doing both now, and I added cookies_tree_model_unittest.cc to this CL.
would you please take another look at it?

it's already >10PM this side, so I'll address any comments tomorrow. enough of
red trees for the day!

On 2010/01/16 02:11:37, bulach wrote:
> Yep, sorry: "logical" monday, not physical.. ;)
> 
> On Fri, Jan 15, 2010 at 6:09 PM, Jeremy Orlow <mailto:jorlow@chromium.org>
wrote:
> > We're not here Monday. &nbsp;How about tues?
> >
> > On Fri, Jan 15, 2010 at 6:08 PM, <mailto:bulach@google.com> wrote:
> >>
> >> thanks Dumi and Jeremy!
> >>
> >> synced, resolved, addressed all the remaining comments...
> >>
> >> please, take another look and let's land this on Monday so that I'll be
> >> around
> >> to watch the bots..
> >>
> >>
> >> http://codereview.chromium.org/523139/diff/7028/6024
> >> File chrome/browser/browsing_data_local_storage_helper.cc (right):
> >>
> >> http://codereview.chromium.org/523139/diff/7028/6024#newcode69
> >> chrome/browser/browsing_data_local_storage_helper.cc:69:
> >> DCHECK(is_fetching_);
> >> On 2010/01/12 23:45:44, dumi wrote:
> >>>
> >>> you're still checking is_fetching_ on the WebKit thread. like jeremy
> >>
> >> said, it's
> >>>
> >>> probably no big deal. just wanted to make sure you meant to leave this
> >>
> >> here
> >>>
> >>> since your comment said it should only be checked/modified on the UI
> >>
> >> thread.
> >>
> >> ops, good catch, thanks!I removed the check, the important checks are in
> >> StartFetching / NotifyInUiThread, both in UI thread.
> >>
> >> http://codereview.chromium.org/523139/diff/7028/6014
> >> File chrome/browser/cookies_tree_model.h (right):
> >>
> >> http://codereview.chromium.org/523139/diff/7028/6014#newcode278
> >> chrome/browser/cookies_tree_model.h:278: const
> >> std::vector<BrowsingDataLocalStorageHelper::LocalStorageInfo>&
> >> On 2010/01/12 23:45:44, dumi wrote:
> >>>
> >>> use the LocalStorageInfoList typedef you added.
> >>
> >> Done.
> >>
> >> http://codereview.chromium.org/523139
> >
> >
>

[jorlow, 2010-01-20 22:30:05]

LGTM

Maybe run gcl try on it again?  Seems like it's trying the previous patch?

http://codereview.chromium.org/523139/diff/10031/10036
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/523139/diff/10031/10036#newcode241
chrome/browser/cookies_tree_model.h:241: Profile* profile,
4 spaces in

[bulach_, 2010-01-21 15:13:51]

the mac cookies view is just in, which means I'll need to update it
along with its tests..
(and there are more linux tests that needs updates as well).

I'm working on this change now, and will update this thread again once
I get the try server green on all platforms.
Apologies for this many rounds, but anyway: it's a great learning exercise.. :)

On Wed, Jan 20, 2010 at 10:30 PM,  <jorlow@chromium.org> wrote:
> LGTM
>
> Maybe run gcl try on it again?  Seems like it's trying the previous patch?
>
>
> http://codereview.chromium.org/523139/diff/10031/10036
> File chrome/browser/cookies_tree_model.h (right):
>
> http://codereview.chromium.org/523139/diff/10031/10036#newcode241
> chrome/browser/cookies_tree_model.h:241: Profile* profile,
> 4 spaces in
>
> http://codereview.chromium.org/523139
>