Adding Application Data dialog for isolated apps

Adding Application Data dialog for isolated apps


BUG=126093
TEST=Unit test, manual testing.


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

[nasko, 2012-06-25 18:08:52]

This is a different way of adding apps cookies UI. Majority of the work comes
out of the previous take at it
(https://chromiumcodereview.appspot.com/10536017/), so it should be mostly
familiar or unchanged.

Owners for review:
chrome/browser/ui/cocoa/content_settings/ -> bauerb@chromium.org
chrome/browser/ui/[gtk,webui], chrome/browser/resources/options2/ ->
estade@chromium.org
chrome/browser/ui -> ben@chromium.org

[tfarina, 2012-06-26 03:36:26]

http://codereview.chromium.org/10636019/diff/10004/chrome/browser/cookies_tre...
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/10636019/diff/10004/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.h:651: public:
nit: add protected virtual destructor.

[markusheintz_, 2012-06-26 16:34:31]

The CookiesTreeModel and LocalDataContainer parts LGTM.
Only one comment and a few nits.

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/coo...
File chrome/browser/cookies_tree_model.cc (right):

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/coo...
chrome/browser/cookies_tree_model.cc:411: // Node doesn't exist, create a new
one and insert it into the (ordered)
nit: Restore the deleted part of the comment.

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/coo...
chrome/browser/cookies_tree_model.cc:464: const std::string
CookieTreeOriginNode::GetUrl() const {
You don't return a URL here.

It looks like this method returns the node name.
Could we rename this method to GetNodeName()?

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/coo...
chrome/browser/cookies_tree_model.cc:714: for (ContainerMap::const_iterator it =
apps_map.begin();
nit: ...::const_iterator it(apps_map.begin());

up to you

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/loc...
File chrome/browser/local_data_container.cc (right):

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/loc...
chrome/browser/local_data_container.cc:14: 
nit: I think you can remove one of the empty lines.

[nasko, 2012-06-26 17:43:20]

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/coo...
File chrome/browser/cookies_tree_model.cc (right):

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/coo...
chrome/browser/cookies_tree_model.cc:411: // Node doesn't exist, create a new
one and insert it into the (ordered)
On 2012/06/26 16:34:31, markusheintz_ wrote:
> nit: Restore the deleted part of the comment.

Done.

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/coo...
chrome/browser/cookies_tree_model.cc:464: const std::string
CookieTreeOriginNode::GetUrl() const {
On 2012/06/26 16:34:31, markusheintz_ wrote:
> You don't return a URL here.
> 
> It looks like this method returns the node name.
> Could we rename this method to GetNodeName()? 

Renamed to GetHost, as it is meant to retrieve the hostname of a URL, not a node
name.

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/coo...
chrome/browser/cookies_tree_model.cc:714: for (ContainerMap::const_iterator it =
apps_map.begin();
On 2012/06/26 16:34:31, markusheintz_ wrote:
> nit: ...::const_iterator it(apps_map.begin());
> 
> up to you

All other for loops in the file use this syntax, so I kept it consistent.

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/coo...
File chrome/browser/cookies_tree_model.h (right):

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/coo...
chrome/browser/cookies_tree_model.h:651: public:
On 2012/06/26 03:36:26, tfarina wrote:
> nit: add protected virtual destructor.

Done.

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/loc...
File chrome/browser/local_data_container.cc (right):

https://chromiumcodereview.appspot.com/10636019/diff/10004/chrome/browser/loc...
chrome/browser/local_data_container.cc:14: 
On 2012/06/26 16:34:31, markusheintz_ wrote:
> nit: I think you can remove one of the empty lines.

Done.

[Bernhard Bauer, 2012-06-26 21:03:55]

https://chromiumcodereview.appspot.com/10636019/diff/8030/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/10636019/diff/8030/chrome/app/generate...
chrome/app/generated_resources.grd:9616: +        Apps cookies and data
Nit: "App cookies and data"?

https://chromiumcodereview.appspot.com/10636019/diff/8030/chrome/browser/cook...
File chrome/browser/cookies_tree_model.cc (right):

https://chromiumcodereview.appspot.com/10636019/diff/8030/chrome/browser/cook...
chrome/browser/cookies_tree_model.cc:438: if (app_id != std::string()) {
Nit: `if (!app_id.empty())`?

https://chromiumcodereview.appspot.com/10636019/diff/8030/chrome/browser/cook...
chrome/browser/cookies_tree_model.cc:717: app_data_map_[it->first] = it->second;
|apps_data_map_| starts out as a copy of |apps_map|, right? Could you just
initialize it as a copy, then iterate over it and only call Init()?

https://chromiumcodereview.appspot.com/10636019/diff/8030/chrome/browser/cook...
File chrome/browser/cookies_tree_model.h (right):

https://chromiumcodereview.appspot.com/10636019/diff/8030/chrome/browser/cook...
chrome/browser/cookies_tree_model.h:677: public CookiesTreeModelDelegate {
Wait, CookiesTreeModel is a CookiesTreeModelDelegate? That doesn't sound right.
It seems like CookiesTreeModel is the only implementation of
CookiesTreeModelDelegate? In that case, why don't you just use CookiesTreeModel
as the type?

https://chromiumcodereview.appspot.com/10636019/diff/8030/chrome/browser/cook...
chrome/browser/cookies_tree_model.h:763: bool use_cookie_source_;
|group_by_cookie_source_|?

https://chromiumcodereview.appspot.com/10636019/diff/8030/chrome/browser/loca...
File chrome/browser/local_data_container.h (right):

https://chromiumcodereview.appspot.com/10636019/diff/8030/chrome/browser/loca...
chrome/browser/local_data_container.h:51: // the empty string, as no app can
have empty id.
Nit: "an empty id".

https://chromiumcodereview.appspot.com/10636019/diff/8030/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

https://chromiumcodereview.appspot.com/10636019/diff/8030/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:285:
CookiesViewCallback callback) {
Could you implement this by constructing the full method name from a method
string parameter and a class name that depends on whether we have an
|app_context_|? Using an enum just for that seems weird (and if you do decide to
stick with the enum, you should name it in DEFINE_STYLE, which will make it even
uglier).

[nasko, 2012-06-26 21:11:46]

It will be great if you can review this soon. I'm trying to land this, so we can
enable a feature on the canary.

The change is very close (in some places identical) to the previous CL you've
seen.

[nasko, 2012-06-26 22:33:16]

http://codereview.chromium.org/10636019/diff/8030/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10636019/diff/8030/chrome/app/generated_resour...
chrome/app/generated_resources.grd:9616: +        Apps cookies and data
On 2012/06/26 21:03:55, Bernhard Bauer wrote:
> Nit: "App cookies and data"?

Done.

http://codereview.chromium.org/10636019/diff/8030/chrome/browser/cookies_tree...
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/10636019/diff/8030/chrome/browser/cookies_tree...
chrome/browser/cookies_tree_model.cc:438: if (app_id != std::string()) {
On 2012/06/26 21:03:55, Bernhard Bauer wrote:
> Nit: `if (!app_id.empty())`?

Done.

http://codereview.chromium.org/10636019/diff/8030/chrome/browser/cookies_tree...
chrome/browser/cookies_tree_model.cc:717: app_data_map_[it->first] = it->second;
On 2012/06/26 21:03:55, Bernhard Bauer wrote:
> |apps_data_map_| starts out as a copy of |apps_map|, right? Could you just
> initialize it as a copy, then iterate over it and only call Init()?

Done.

http://codereview.chromium.org/10636019/diff/8030/chrome/browser/cookies_tree...
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/10636019/diff/8030/chrome/browser/cookies_tree...
chrome/browser/cookies_tree_model.h:677: public CookiesTreeModelDelegate {
On 2012/06/26 21:03:55, Bernhard Bauer wrote:
> Wait, CookiesTreeModel is a CookiesTreeModelDelegate? That doesn't sound
right.
> It seems like CookiesTreeModel is the only implementation of
> CookiesTreeModelDelegate? In that case, why don't you just use
CookiesTreeModel
> as the type?

Done.

http://codereview.chromium.org/10636019/diff/8030/chrome/browser/cookies_tree...
chrome/browser/cookies_tree_model.h:763: bool use_cookie_source_;
On 2012/06/26 21:03:55, Bernhard Bauer wrote:
> |group_by_cookie_source_|?

This was just moved around, but I think it is a good rename. Done.

http://codereview.chromium.org/10636019/diff/8030/chrome/browser/local_data_c...
File chrome/browser/local_data_container.h (right):

http://codereview.chromium.org/10636019/diff/8030/chrome/browser/local_data_c...
chrome/browser/local_data_container.h:51: // the empty string, as no app can
have empty id.
On 2012/06/26 21:03:55, Bernhard Bauer wrote:
> Nit: "an empty id".

Done.

[Evan Stade, 2012-06-27 04:53:00]

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
File chrome/browser/resources/options2/app_cookies_view.css (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
chrome/browser/resources/options2/app_cookies_view.css:6: #app-cookies-view-page
{
do not just copy cookies_view.css wholesale. Share the parts that you can share,
which seems to be most of it.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
File chrome/browser/resources/options2/app_cookies_view.js (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
chrome/browser/resources/options2/app_cookies_view.js:5: cr.define('options',
function() {
likewise, don't just copy cookies_view.js and make some minor tweaks...

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
File chrome/browser/resources/options2/cookies_list.js (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
chrome/browser/resources/options2/cookies_list.js:127: if
(this.origin.data.appId && this.origin.data.appId !== '') {
'' is falsey so the second half of this check is not necessary.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:307: if (app_context_)
ternary operator.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.h (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.h:72: // Return the
proper callback string, depending on the context in which the
"the context in which the view operates" is not a phrase that I can understand.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.h:74: const std::string
GetCallback(CookiesViewCallback callback);
const without ref is meaningless for return types. remove the const.

[nasko, 2012-06-27 17:17:01]

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
File chrome/browser/resources/options2/app_cookies_view.css (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
chrome/browser/resources/options2/app_cookies_view.css:6: #app-cookies-view-page
{
On 2012/06/27 04:53:00, Evan Stade wrote:
> do not just copy cookies_view.css wholesale. Share the parts that you can
share,
> which seems to be most of it.

I've removed some of the redundancy, though not all, as the UI is expected to
diverge. See the other comment.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
File chrome/browser/resources/options2/app_cookies_view.js (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
chrome/browser/resources/options2/app_cookies_view.js:5: cr.define('options',
function() {
On 2012/06/27 04:53:00, Evan Stade wrote:
> likewise, don't just copy cookies_view.js and make some minor tweaks...

Since the CookiesView is a singleton, I've made a copy, since going forward this
UI will likely diverge into looking different than this. There has been an
agreement with UI leads and Mike West for initial copy of the page, which will
be adjusted later.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
File chrome/browser/resources/options2/cookies_list.js (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
chrome/browser/resources/options2/cookies_list.js:127: if
(this.origin.data.appId && this.origin.data.appId !== '') {
On 2012/06/27 04:53:00, Evan Stade wrote:
> '' is falsey so the second half of this check is not necessary.

The second half is supposed to check if the appId is an empty string or not. I
can see in the debugger that it does evaluate differently, so the proper class
is set on the object.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:307: if (app_context_)
On 2012/06/27 04:53:00, Evan Stade wrote:
> ternary operator.

Done.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.h (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.h:72: // Return the
proper callback string, depending on the context in which the
On 2012/06/27 04:53:00, Evan Stade wrote:
> "the context in which the view operates" is not a phrase that I can
understand.

Done.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.h:74: const std::string
GetCallback(CookiesViewCallback callback);
On 2012/06/27 04:53:00, Evan Stade wrote:
> const without ref is meaningless for return types. remove the const.

Done.

[Evan Stade, 2012-06-28 04:15:13]

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
File chrome/browser/resources/options2/app_cookies_view.css (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
chrome/browser/resources/options2/app_cookies_view.css:6: #app-cookies-view-page
{
On 2012/06/27 17:17:01, nasko wrote:
> On 2012/06/27 04:53:00, Evan Stade wrote:
> > do not just copy cookies_view.css wholesale. Share the parts that you can
> share,
> > which seems to be most of it.
> 
> I've removed some of the redundancy, though not all, as the UI is expected to
> diverge. See the other comment.

I would rather you share everything, then override what makes sense to override,
because the path you are on is almost guaranteed to end in a state where you
have undesirable copypasta.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
File chrome/browser/resources/options2/app_cookies_view.js (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
chrome/browser/resources/options2/app_cookies_view.js:5: cr.define('options',
function() {
On 2012/06/27 17:17:01, nasko wrote:
> On 2012/06/27 04:53:00, Evan Stade wrote:
> > likewise, don't just copy cookies_view.js and make some minor tweaks...
> 
> Since the CookiesView is a singleton, I've made a copy, since going forward
this
> UI will likely diverge into looking different than this. There has been an
> agreement with UI leads and Mike West for initial copy of the page, which will
> be adjusted later.

it would be really easy to use inheritance to share and also allow differences.
I don't see why it is relevant that CookiesView is a singleton.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
File chrome/browser/resources/options2/cookies_list.js (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
chrome/browser/resources/options2/cookies_list.js:127: if
(this.origin.data.appId && this.origin.data.appId !== '') {
for which value of this.origin.data.appId does [1] behave differently than [2]

[1] if (this.origin.data.appId && this.origin.data.appId !== '')
[2] if (this.origin.data.appId)

[nasko, 2012-06-29 00:46:52]

Thanks a lot Evan for pointing me in the right direction!

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
File chrome/browser/resources/options2/app_cookies_view.css (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
chrome/browser/resources/options2/app_cookies_view.css:6: #app-cookies-view-page
{
On 2012/06/28 04:15:13, Evan Stade wrote:
> On 2012/06/27 17:17:01, nasko wrote:
> > On 2012/06/27 04:53:00, Evan Stade wrote:
> > > do not just copy cookies_view.css wholesale. Share the parts that you can
> > share,
> > > which seems to be most of it.
> > 
> > I've removed some of the redundancy, though not all, as the UI is expected
to
> > diverge. See the other comment.
> 
> I would rather you share everything, then override what makes sense to
override,
> because the path you are on is almost guaranteed to end in a state where you
> have undesirable copypasta.

Done.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
File chrome/browser/resources/options2/app_cookies_view.js (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
chrome/browser/resources/options2/app_cookies_view.js:5: cr.define('options',
function() {
On 2012/06/28 04:15:13, Evan Stade wrote:
> On 2012/06/27 17:17:01, nasko wrote:
> > On 2012/06/27 04:53:00, Evan Stade wrote:
> > > likewise, don't just copy cookies_view.js and make some minor tweaks...
> > 
> > Since the CookiesView is a singleton, I've made a copy, since going forward
> this
> > UI will likely diverge into looking different than this. There has been an
> > agreement with UI leads and Mike West for initial copy of the page, which
will
> > be adjusted later.
> 
> it would be really easy to use inheritance to share and also allow
differences.
> I don't see why it is relevant that CookiesView is a singleton.

Done.

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
File chrome/browser/resources/options2/cookies_list.js (right):

http://codereview.chromium.org/10636019/diff/19002/chrome/browser/resources/o...
chrome/browser/resources/options2/cookies_list.js:127: if
(this.origin.data.appId && this.origin.data.appId !== '') {
On 2012/06/28 04:15:13, Evan Stade wrote:
> for which value of this.origin.data.appId does [1] behave differently than [2]
> 
> [1] if (this.origin.data.appId && this.origin.data.appId !== '')
> [2] if (this.origin.data.appId)

Done.

[nasko, 2012-07-02 14:28:10]

ping

[Ben Goodger (Google), 2012-07-02 18:01:16]

browser/ui/views lgtm

[Evan Stade, 2012-07-02 18:15:37]

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/res...
File chrome/browser/resources/options2/cookies_list.js (right):

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/res...
chrome/browser/resources/options2/cookies_list.js:132: if
(this.origin.data.appId !== '') {
if (this.origin.data.appId)

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/res...
chrome/browser/resources/options2/cookies_list.js:133: this.siteChild.className
+= ' app-cookie-site';
classList.add

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/res...
File chrome/browser/resources/options2/cookies_view.html (right):

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/res...
chrome/browser/resources/options2/cookies_view.html:13: <div
id="cookies-header-controls" class="cookies-header-controls">
please remove the ids that are no longer necessary

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/ui/...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:160:
web_ui()->CallJavascriptFunction(GetCallback(onTreeItemRemoved), args);
this is not right, the js call should depend on the model, not the "context"

[nasko, 2012-07-02 20:48:40]

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/res...
File chrome/browser/resources/options2/cookies_list.js (right):

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/res...
chrome/browser/resources/options2/cookies_list.js:132: if
(this.origin.data.appId !== '') {
On 2012/07/02 18:15:37, Evan Stade wrote:
> if (this.origin.data.appId)

Done.

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/res...
chrome/browser/resources/options2/cookies_list.js:133: this.siteChild.className
+= ' app-cookie-site';
On 2012/07/02 18:15:37, Evan Stade wrote:
> classList.add

Done.

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/res...
File chrome/browser/resources/options2/cookies_view.html (right):

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/res...
chrome/browser/resources/options2/cookies_view.html:13: <div
id="cookies-header-controls" class="cookies-header-controls">
On 2012/07/02 18:15:37, Evan Stade wrote:
> please remove the ids that are no longer necessary

Done.

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/ui/...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:160:
web_ui()->CallJavascriptFunction(GetCallback(onTreeItemRemoved), args);
On 2012/07/02 18:15:37, Evan Stade wrote:
> this is not right, the js call should depend on the model, not the "context"

That would mean adding a new model that will do almost everything identically.
Subclassing the current model to inherit the functionality (to avoid
duplication) won't really buy much, as almost every function in this class is
touched. I will have to do a similar approach of having a method that returns
which JS callback to call, based on the type of the model.

[Evan Stade, 2012-07-02 21:04:02]

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

https://chromiumcodereview.appspot.com/10636019/diff/25003/chrome/browser/ui/...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:160:
web_ui()->CallJavascriptFunction(GetCallback(onTreeItemRemoved), args);
On 2012/07/02 20:48:40, nasko wrote:
> On 2012/07/02 18:15:37, Evan Stade wrote:
> > this is not right, the js call should depend on the model, not the "context"
> 
> That would mean adding a new model that will do almost everything identically.
> Subclassing the current model to inherit the functionality (to avoid
> duplication) won't really buy much, as almost every function in this class is
> touched.

I don't understand the above

> I will have to do a similar approach of having a method that returns
> which JS callback to call, based on the type of the model.

this is almost what I'm suggesting, except instead of "type of the model", use
"which model instance"

[nasko, 2012-07-02 21:49:15]

http://codereview.chromium.org/10636019/diff/25003/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

http://codereview.chromium.org/10636019/diff/25003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:160:
web_ui()->CallJavascriptFunction(GetCallback(onTreeItemRemoved), args);
On 2012/07/02 21:04:02, Evan Stade wrote:
> On 2012/07/02 20:48:40, nasko wrote:
> > On 2012/07/02 18:15:37, Evan Stade wrote:
> > > this is not right, the js call should depend on the model, not the
"context"
> > 
> > That would mean adding a new model that will do almost everything
identically.
> > Subclassing the current model to inherit the functionality (to avoid
> > duplication) won't really buy much, as almost every function in this class
is
> > touched.
> 
> I don't understand the above
> 
> > I will have to do a similar approach of having a method that returns
> > which JS callback to call, based on the type of the model.
> 
> this is almost what I'm suggesting, except instead of "type of the model", use
> "which model instance"

Done.

[Bernhard Bauer, 2012-07-03 17:41:12]

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/cookies_tre...
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.cc:39: CookieTreeOriginNode* origin =
static_cast<CookieTreeOriginNode*>(
Could you also add a node_type CHECK like below?

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.cc:77: if (container) {
Can |container| ever be NULL?

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.cc:101: const CookieTreeOriginNode* ltn =
static_cast<const CookieTreeOriginNode*>(
Nit: Break after "="?

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.cc:845: }
This seems like a lot of boilerplate code that's repeated for every data type.
Maybe we could simplify this?

For example, I noticed that inside of this class, these methods are only called
with a NULL |app_id|, which means we iterate over all apps. From
LocalDataContainer, we always pass in a specific ID, so we only update data for
that app. So, maybe we could do the iterating over all apps in one big loop in
UpdateSearchResults() instead, and have these methods only apply to a single
app? We might need to figure out what to do about the batch notifications
though.

[Evan Stade, 2012-07-03 18:11:28]

should this be developed behind a flag?

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/resources/o...
File chrome/browser/resources/options2/cookies_view_app.js (right):

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/resources/o...
chrome/browser/resources/options2/cookies_view_app.js:8: // AppCookiesView
class:
name of the file should match name of the class

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/resources/o...
chrome/browser/resources/options2/cookies_view_app.js:48: 
remove extra line

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:284:
web_ui()->CallJavascriptFunction("AppCookiesView.loadChildren", args);
why not use a helper function like what you had before

std::string GetViewName(const std::string& model) {
  return model == app_cookies_tree_model_ ?
      "AppCookiesView" : "CookiesView";
}

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.h (right):

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.h:43: onTreeItemAdded,
don't see where this is used

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.h:87: // Flag to indicate
whether the view is used in the apps dialog context or
I don't think this makes much sense. Can you rephrase it? Something like:

a flag to indicate which view is active...

[nasko, 2012-07-09 17:07:06]

I've refactored some of the code in the cookie tree model per discussion with
Bernhard.

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/cookies_tre...
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.cc:39: CookieTreeOriginNode* origin =
static_cast<CookieTreeOriginNode*>(
On 2012/07/03 17:41:12, Bernhard Bauer wrote:
> Could you also add a node_type CHECK like below?

Done.

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.cc:77: if (container) {
On 2012/07/03 17:41:12, Bernhard Bauer wrote:
> Can |container| ever be NULL?

Done.

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.cc:101: const CookieTreeOriginNode* ltn =
static_cast<const CookieTreeOriginNode*>(
On 2012/07/03 17:41:12, Bernhard Bauer wrote:
> Nit: Break after "="?

Done.

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.cc:845: }
On 2012/07/03 17:41:12, Bernhard Bauer wrote:
> This seems like a lot of boilerplate code that's repeated for every data type.
> Maybe we could simplify this?
> 
> For example, I noticed that inside of this class, these methods are only
called
> with a NULL |app_id|, which means we iterate over all apps. From
> LocalDataContainer, we always pass in a specific ID, so we only update data
for
> that app. So, maybe we could do the iterating over all apps in one big loop in
> UpdateSearchResults() instead, and have these methods only apply to a single
> app? We might need to figure out what to do about the batch notifications
> though.

Done.

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/resources/o...
File chrome/browser/resources/options2/cookies_view_app.js (right):

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/resources/o...
chrome/browser/resources/options2/cookies_view_app.js:8: // AppCookiesView
class:
On 2012/07/03 18:11:28, Evan Stade wrote:
> name of the file should match name of the class

Done.

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/resources/o...
chrome/browser/resources/options2/cookies_view_app.js:48: 
On 2012/07/03 18:11:28, Evan Stade wrote:
> remove extra line

Done.

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:284:
web_ui()->CallJavascriptFunction("AppCookiesView.loadChildren", args);
On 2012/07/03 18:11:28, Evan Stade wrote:
> why not use a helper function like what you had before
> 
> std::string GetViewName(const std::string& model) {
>   return model == app_cookies_tree_model_ ?
>       "AppCookiesView" : "CookiesView";
> }

Done.

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.h (right):

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.h:43: onTreeItemAdded,
On 2012/07/03 18:11:28, Evan Stade wrote:
> don't see where this is used

Re-added the common function, so it is now used.

http://codereview.chromium.org/10636019/diff/30011/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.h:87: // Flag to indicate
whether the view is used in the apps dialog context or
On 2012/07/03 18:11:28, Evan Stade wrote:
> I don't think this makes much sense. Can you rephrase it? Something like:
> 
> a flag to indicate which view is active...
> 

Done.

[Bernhard Bauer, 2012-07-09 17:34:47]

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/cookies_tre...
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.cc:818: notifier.StartBatchUpdate();
This is probably not necessary anymore.

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/cookies_tre...
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.h:731: ScopedBatchUpdateNotifier& notifier,
Could you pass in |notifier| as a pointer?

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:281:
GetCallback(loadChildren, GetTreeModel()), args);
Passing in |loadChildren| like this still looks a bit weird (it doesn't fit the
enum style, for example). As each of these is only used once anyway, could you
just pass in the method name as a string?

[nasko, 2012-07-09 18:17:34]

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/cookies_tre...
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.cc:818: notifier.StartBatchUpdate();
On 2012/07/09 17:34:47, Bernhard Bauer wrote:
> This is probably not necessary anymore.

The Remove call does notification updates on its own, so while we might not need
it here, I'd like to keep parity with how the old version functions.

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/cookies_tre...
File chrome/browser/cookies_tree_model.h (right):

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.h:731: ScopedBatchUpdateNotifier& notifier,
On 2012/07/09 17:34:47, Bernhard Bauer wrote:
> Could you pass in |notifier| as a pointer?

Done.

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:281:
GetCallback(loadChildren, GetTreeModel()), args);
On 2012/07/09 17:34:47, Bernhard Bauer wrote:
> Passing in |loadChildren| like this still looks a bit weird (it doesn't fit
the
> enum style, for example). As each of these is only used once anyway, could you
> just pass in the method name as a string?

Done.

[nasko, 2012-07-10 14:15:51]

I am moving office at the end of the week, so I need to commit this change as
soon as possible. I would appreciate your timely reviews, as the changes are
minor between successive patches.
Thanks!

[Bernhard Bauer, 2012-07-10 18:18:03]

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/cookies_tre...
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/10636019/diff/40001/chrome/browser/cookies_tre...
chrome/browser/cookies_tree_model.cc:818: notifier.StartBatchUpdate();
On 2012/07/09 18:17:34, nasko wrote:
> On 2012/07/09 17:34:47, Bernhard Bauer wrote:
> > This is probably not necessary anymore.
> 
> The Remove call does notification updates on its own, so while we might not
need
> it here, I'd like to keep parity with how the old version functions.

Ok, fair enough.

http://codereview.chromium.org/10636019/diff/48002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

http://codereview.chromium.org/10636019/diff/48002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:180: void
CookiesViewHandler::EnsureCookiesTreeModelCreated() {
Hmm, it looks like exactly one of |app_cookies_tree_model_| and
|cookies_tree_model_| is non-NULL, right? If so, it would make sense to merge
the two fields. Then, we could merge the two if-statements here, which have a
lot of common code.

http://codereview.chromium.org/10636019/diff/48002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:211: // appropriate
browsing data helper objects to the consutrctor.
Nit: constructor

http://codereview.chromium.org/10636019/diff/48002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:298: std::string
method, CookiesTreeModel* model) {
You're always calling this method with GetTreeModel(), which looks at
|app_context_| in turn, so you could just leave out the parameter, and directly
look at |app_context_|.

http://codereview.chromium.org/10636019/diff/48002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:301: if (model ==
app_cookies_tree_model_) {
Nit: braces are unnecessary.

[Bernhard Bauer, 2012-07-10 19:36:58]

After off-CL discussion, LGTM with the two nits from my previous mail addressed.

[nasko, 2012-07-10 19:54:10]

http://codereview.chromium.org/10636019/diff/48002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right):

http://codereview.chromium.org/10636019/diff/48002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:180: void
CookiesViewHandler::EnsureCookiesTreeModelCreated() {
On 2012/07/10 18:18:03, Bernhard Bauer wrote:
> Hmm, it looks like exactly one of |app_cookies_tree_model_| and
> |cookies_tree_model_| is non-NULL, right? If so, it would make sense to merge
> the two fields. Then, we could merge the two if-statements here, which have a
> lot of common code.

There is one CookiesViewHandler for the lifetime of the Settings page, so both
pointers can be non-NULL.

http://codereview.chromium.org/10636019/diff/48002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:211: // appropriate
browsing data helper objects to the consutrctor.
On 2012/07/10 18:18:03, Bernhard Bauer wrote:
> Nit: constructor

Done.

http://codereview.chromium.org/10636019/diff/48002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:298: std::string
method, CookiesTreeModel* model) {
On 2012/07/10 18:18:03, Bernhard Bauer wrote:
> You're always calling this method with GetTreeModel(), which looks at
> |app_context_| in turn, so you could just leave out the parameter, and
directly
> look at |app_context_|.

This should be acting on the model. I've changed the TreeNodesAdded/Removed to
pass the model parameter, not the GetTreeModel() one, as it was pointed in
earlier review by Evan.

http://codereview.chromium.org/10636019/diff/48002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/cookies_view_handler2.cc:301: if (model ==
app_cookies_tree_model_) {
On 2012/07/10 18:18:03, Bernhard Bauer wrote:
> Nit: braces are unnecessary.

Done.

[Evan Stade, 2012-07-11 01:50:37]

lgtm

[commit-bot: I haz the power, 2012-07-11 17:15:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/10636019/24008

[commit-bot: I haz the power, 2012-07-11 17:15:36]

Failed to apply patch for chrome/browser/cookies_tree_model.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/cookies_tree_model.cc
Hunk #1 succeeded at 91 with fuzz 2 (offset 62 lines).
Hunk #2 succeeded at 135 (offset 62 lines).
Hunk #3 FAILED at 89.
Hunk #4 FAILED at 106.
Hunk #5 succeeded at 160 (offset 3 lines).
Hunk #6 succeeded at 189 (offset 3 lines).
Hunk #7 succeeded at 216 (offset 3 lines).
Hunk #8 succeeded at 244 (offset 3 lines).
Hunk #9 succeeded at 271 (offset 3 lines).
Hunk #10 succeeded at 299 (offset 3 lines).
Hunk #11 succeeded at 327 (offset 3 lines).
Hunk #12 succeeded at 354 (offset 3 lines).
Hunk #13 FAILED at 376.
Hunk #14 FAILED at 410.
Hunk #15 succeeded at 428 (offset 2 lines).
Hunk #16 succeeded at 663 (offset -12 lines).
Hunk #17 succeeded at 770 (offset -12 lines).
Hunk #18 succeeded at 806 (offset -12 lines).
Hunk #19 succeeded at 883 (offset -12 lines).
Hunk #20 succeeded at 907 (offset -12 lines).
Hunk #21 succeeded at 1150 (offset -12 lines).
4 out of 21 hunks FAILED -- saving rejects to file
chrome/browser/cookies_tree_model.cc.rej

[commit-bot: I haz the power, 2012-07-11 18:49:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/10636019/51002

[commit-bot: I haz the power, 2012-07-11 20:14:19]

Change committed as 146186

[markusheintz_, 2012-07-11 21:55:19]

On 2012/07/11 20:14:19, I haz the power (commit-bot) wrote:
> Change committed as 146186

LGTM patchset 11. Thanks for this CL.