Show Flash LSOs for Pepper Flash in cookie dialog.

Show Flash LSOs for Pepper Flash in cookie dialog.

It won't show anything until the Flash side has been updated.

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

[Bernhard Bauer, 2012-07-28 04:01:30]

Please review.

Markus: content settings and browsing data
James: WebUI

Thanks!

[James Hawkins, 2012-07-30 03:04:39]

LGTM with one nit and two doleful comments.

https://chromiumcodereview.appspot.com/10790150/diff/7028/chrome/browser/brow...
File chrome/browser/browsing_data/browsing_data_flash_lso_helper.h (right):

https://chromiumcodereview.appspot.com/10790150/diff/7028/chrome/browser/brow...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.h:18: class
BrowsingDataFlashLSOHelper
nit: Document class.

https://chromiumcodereview.appspot.com/10790150/diff/7028/chrome/browser/ui/w...
File chrome/browser/ui/webui/cookies_tree_model_util.cc (right):

https://chromiumcodereview.appspot.com/10790150/diff/7028/chrome/browser/ui/w...
chrome/browser/ui/webui/cookies_tree_model_util.cc:90: bool
CookiesTreeModelUtil::GetCookieTreeNodeDictionary(
Holy moly this existing code is bad.

https://chromiumcodereview.appspot.com/10790150/diff/7028/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/cookies_view_handler.cc (right):

https://chromiumcodereview.appspot.com/10790150/diff/7028/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/cookies_view_handler.cc:224: NULL, NULL, NULL,
NULL, NULL, NULL, NULL, NULL, NULL);
Super sad face.

[markusheintz_, 2012-07-30 14:02:14]

Do also plan to add a Canned helper ?

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:16: class
BrowsingDataFlashLSOHelperImpl
Currently there is only one implementation. Do you plan to add one more
implementation for a canned version?

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:27: //
PepperFlashSettingsManager::Client:
bike shedding: "Overridden from PepperFlash..."

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:35: private:
Please document the attributes.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:44:
GetSitesWithDataCallback callback_;
Should we initialize this to NULL?

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:82: bool success)
{
nit: Please fix indentation.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_flash_lso_helper.h (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.h:18: class
BrowsingDataFlashLSOHelper
On 2012/07/30 03:04:39, James Hawkins wrote:
> nit: Document class.

+1 :)

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.h:22:
GetSitesWithDataCallback;
nit: hm ... maybe "GetSitesWithFlashDataCallback". I'll leave it up to you.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/local_data_container.h (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/local_data_container.h:75:
BrowsingDataFlashLSOHelper* flash_data_getter);
nit: s/getter/helper/

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/content_sett...
File chrome/browser/content_settings/local_shared_objects_container.cc (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/content_sett...
chrome/browser/content_settings/local_shared_objects_container.cc:184:
scoped_ptr<CookiesTreeModel>
Cool thanks.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/content_sett...
File chrome/browser/content_settings/local_shared_objects_container.h (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/content_sett...
chrome/browser/content_settings/local_shared_objects_container.h:36: // Creates
a new CookiesTreeModel for all objects in the container.
Could you mention that the objects are copied? Thanks

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/ui/gtk/colle...
File chrome/browser/ui/gtk/collected_cookies_gtk.cc (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/ui/gtk/colle...
chrome/browser/ui/gtk/collected_cookies_gtk.cc:216: const
LocalSharedObjectsContainer& allowed =
Although 'allowed_lsos' is already a bad name this change does not really
improve the name :(.

Maybe something like 'allowed_site_data' or just 'allowed_data' would be better.

same in all other places where you s/allowed_lsos/allowed/
same for blocked_...

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/cookies_view_handler.cc (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/cookies_view_handler.cc:224: NULL, NULL, NULL,
NULL, NULL, NULL, NULL, NULL, NULL);
On 2012/07/30 03:04:39, James Hawkins wrote:
> Super sad face.

The cookies tree mode and browsing data helper code is in a really bad shape. I
REALLY want to change this over time.

In a first step I moved all browsing data helper stuff into a separate dir. Now
I will improve the code little by little (as time and other high priority tasks
permits). I hope to turn a lot of sad faces into happy faces (including my own).

[Bernhard Bauer, 2012-07-31 00:20:03]

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:16: class
BrowsingDataFlashLSOHelperImpl
On 2012/07/30 14:02:15, markusheintz_ wrote:
> Currently there is only one implementation. Do you plan to add one more
> implementation for a canned version?

Right now content settings aren't hooked up yet to Flash, so we can't block
LSOs, nor are we notified when they're accessed. Once
http://codereview.chromium.org/10831045/ has landed, we could register
allow/block decisions in TabSpecificContentSettings though (which isn't fully
correct, as we don't get the LSO domain, but rather the URL of the swf file that
tries to access it, but it's close enough).

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:27: //
PepperFlashSettingsManager::Client:
On 2012/07/30 14:02:15, markusheintz_ wrote:
> bike shedding: "Overridden from PepperFlash..."

Done.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:35: private:
On 2012/07/30 14:02:15, markusheintz_ wrote:
> Please document the attributes.

Done.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:44:
GetSitesWithDataCallback callback_;
On 2012/07/30 14:02:15, markusheintz_ wrote:
> Should we initialize this to NULL?

The default constructor for base::Callback already initializes everything to
defined values.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:82: bool success)
{
On 2012/07/30 14:02:15, markusheintz_ wrote:
> nit: Please fix indentation.

Done.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_flash_lso_helper.h (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.h:18: class
BrowsingDataFlashLSOHelper
On 2012/07/30 03:04:39, James Hawkins wrote:
> nit: Document class.

Done.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.h:22:
GetSitesWithDataCallback;
On 2012/07/30 14:02:15, markusheintz_ wrote:
> nit: hm ... maybe "GetSitesWithFlashDataCallback". I'll leave it up to you.

Done.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/local_data_container.h (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/browsing_dat...
chrome/browser/browsing_data/local_data_container.h:75:
BrowsingDataFlashLSOHelper* flash_data_getter);
On 2012/07/30 14:02:15, markusheintz_ wrote:
> nit: s/getter/helper/

Done.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/content_sett...
File chrome/browser/content_settings/local_shared_objects_container.h (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/content_sett...
chrome/browser/content_settings/local_shared_objects_container.h:36: // Creates
a new CookiesTreeModel for all objects in the container.
On 2012/07/30 14:02:15, markusheintz_ wrote:
> Could you mention that the objects are copied? Thanks

Done.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/ui/gtk/colle...
File chrome/browser/ui/gtk/collected_cookies_gtk.cc (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/ui/gtk/colle...
chrome/browser/ui/gtk/collected_cookies_gtk.cc:216: const
LocalSharedObjectsContainer& allowed =
On 2012/07/30 14:02:15, markusheintz_ wrote:
> Although 'allowed_lsos' is already a bad name this change does not really
> improve the name :(.
> 
> Maybe something like 'allowed_site_data' or just 'allowed_data' would be
better.
> 
> same in all other places where you s/allowed_lsos/allowed/
> same for blocked_...

Done.

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/cookies_view_handler.cc (right):

http://codereview.chromium.org/10790150/diff/7028/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/cookies_view_handler.cc:224: NULL, NULL, NULL,
NULL, NULL, NULL, NULL, NULL, NULL);
On 2012/07/30 03:04:39, James Hawkins wrote:
> Super sad face.

Markus, we should add doing something like having a small constructor with
setters to the list of things to clean up for the browsing data helpers.

[markusheintz_, 2012-07-31 15:09:13]

Only nits: Could you please make the naming consistent and replace domain[s]
with site[s].

https://chromiumcodereview.appspot.com/10790150/diff/7028/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/cookies_view_handler.cc (right):

https://chromiumcodereview.appspot.com/10790150/diff/7028/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/cookies_view_handler.cc:224: NULL, NULL, NULL,
NULL, NULL, NULL, NULL, NULL, NULL);
On 2012/07/31 00:20:03, Bernhard Bauer wrote:
> On 2012/07/30 03:04:39, James Hawkins wrote:
> > Super sad face.
> 
> Markus, we should add doing something like having a small constructor with
> setters to the list of things to clean up for the browsing data helpers.

Yes, I'll file a bug.

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
File chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc (right):

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:31: const
std::vector<std::string>& sites) OVERRIDE;
Question: By sites we mean origins, right?

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
File chrome/browser/browsing_data/local_data_container.cc (right):

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
chrome/browser/browsing_data/local_data_container.cc:196: flash_lso_domain_list_
= domains;
domains/sites/
domain/site/

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
File chrome/browser/browsing_data/local_data_container.h (right):

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
chrome/browser/browsing_data/local_data_container.h:51: typedef
std::vector<std::string> FlashLSODomainList;
You use the term "site" instead of "domain" in
"browsing_data_flash_lso_helper.*" Could you replace domain with site here and
elsewhere?

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
chrome/browser/browsing_data/local_data_container.h:112: void
OnFlashLSOInfoLoaded(const FlashLSODomainList& domains);
s/domains/sites/

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
chrome/browser/browsing_data/local_data_container.h:142: FlashLSODomainList
flash_lso_domain_list_;
domain/site/

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
File chrome/browser/browsing_data/mock_browsing_data_flash_lso_helper.cc
(right):

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
chrome/browser/browsing_data/mock_browsing_data_flash_lso_helper.cc:29:
domains_.push_back(domain);
s/domain/site/

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
File chrome/browser/browsing_data/mock_browsing_data_flash_lso_helper.h (right):

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
chrome/browser/browsing_data/mock_browsing_data_flash_lso_helper.h:24: void
AddFlashLSODomain(const std::string& domain);
s/AddFlashLSODomain/AddFlashLSOSite/
s/domain/site

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
chrome/browser/browsing_data/mock_browsing_data_flash_lso_helper.h:37:
std::vector<std::string> domains_;
s/domains_/sites_

[markusheintz_, 2012-07-31 15:09:53]

LGTM after fixing the nits.

On 2012/07/31 15:09:13, markusheintz_ wrote:
> Only nits: Could you please make the naming consistent and replace domain[s]
> with site[s].
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/7028/chrome/browser/ui/w...
> File chrome/browser/ui/webui/options2/cookies_view_handler.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/7028/chrome/browser/ui/w...
> chrome/browser/ui/webui/options2/cookies_view_handler.cc:224: NULL, NULL,
NULL,
> NULL, NULL, NULL, NULL, NULL, NULL);
> On 2012/07/31 00:20:03, Bernhard Bauer wrote:
> > On 2012/07/30 03:04:39, James Hawkins wrote:
> > > Super sad face.
> > 
> > Markus, we should add doing something like having a small constructor with
> > setters to the list of things to clean up for the browsing data helpers.
> 
> Yes, I'll file a bug.
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> File chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:31: const
> std::vector<std::string>& sites) OVERRIDE;
> Question: By sites we mean origins, right?
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> File chrome/browser/browsing_data/local_data_container.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> chrome/browser/browsing_data/local_data_container.cc:196:
flash_lso_domain_list_
> = domains;
> domains/sites/
> domain/site/
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> File chrome/browser/browsing_data/local_data_container.h (right):
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> chrome/browser/browsing_data/local_data_container.h:51: typedef
> std::vector<std::string> FlashLSODomainList;
> You use the term "site" instead of "domain" in
> "browsing_data_flash_lso_helper.*" Could you replace domain with site here and
> elsewhere?
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> chrome/browser/browsing_data/local_data_container.h:112: void
> OnFlashLSOInfoLoaded(const FlashLSODomainList& domains);
> s/domains/sites/
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> chrome/browser/browsing_data/local_data_container.h:142: FlashLSODomainList
> flash_lso_domain_list_;
> domain/site/
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> File chrome/browser/browsing_data/mock_browsing_data_flash_lso_helper.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> chrome/browser/browsing_data/mock_browsing_data_flash_lso_helper.cc:29:
> domains_.push_back(domain);
> s/domain/site/
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> File chrome/browser/browsing_data/mock_browsing_data_flash_lso_helper.h
(right):
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> chrome/browser/browsing_data/mock_browsing_data_flash_lso_helper.h:24: void
> AddFlashLSODomain(const std::string& domain);
> s/AddFlashLSODomain/AddFlashLSOSite/
> s/domain/site
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> chrome/browser/browsing_data/mock_browsing_data_flash_lso_helper.h:37:
> std::vector<std::string> domains_;
> s/domains_/sites_

[Bernhard Bauer, 2012-07-31 19:16:59]

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
File chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc (right):

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:31: const
std::vector<std::string>& sites) OVERRIDE;
On 2012/07/31 15:09:13, markusheintz_ wrote:
> Question: By sites we mean origins, right?

No. Flash stores LSOs based on domain, not on origin.

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
File chrome/browser/browsing_data/local_data_container.h (right):

https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
chrome/browser/browsing_data/local_data_container.h:51: typedef
std::vector<std::string> FlashLSODomainList;
On 2012/07/31 15:09:13, markusheintz_ wrote:
> You use the term "site" instead of "domain" in
> "browsing_data_flash_lso_helper.*" Could you replace domain with site here and
> elsewhere?

I'm actually doing that on purpose. "Site" is the term used by the ClearSiteData
API (see https://wiki.mozilla.org/NPAPI:ClearSiteData[1]), so I'm using that
term in all classes that are essentially a wrapper around that API. However,
"site" is a somewhat fuzzy term, and the spec says that it actually means domain
(and Flash also keys LSOs on the domain), so I'm using "domain" in the cookie
tree model where the semantic actually matters.

[1] I'm not using the NPAPI version, but I tried to keep the Pepper version as
similar as possible.

[markusheintz_, 2012-08-01 08:44:17]

On 2012/07/31 19:16:59, Bernhard Bauer wrote:
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> File chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> chrome/browser/browsing_data/browsing_data_flash_lso_helper.cc:31: const
> std::vector<std::string>& sites) OVERRIDE;
> On 2012/07/31 15:09:13, markusheintz_ wrote:
> > Question: By sites we mean origins, right?
> 
> No. Flash stores LSOs based on domain, not on origin.
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> File chrome/browser/browsing_data/local_data_container.h (right):
> 
>
https://chromiumcodereview.appspot.com/10790150/diff/19004/chrome/browser/bro...
> chrome/browser/browsing_data/local_data_container.h:51: typedef
> std::vector<std::string> FlashLSODomainList;
> On 2012/07/31 15:09:13, markusheintz_ wrote:
> > You use the term "site" instead of "domain" in
> > "browsing_data_flash_lso_helper.*" Could you replace domain with site here
and
> > elsewhere?
> 
> I'm actually doing that on purpose. "Site" is the term used by the
ClearSiteData
> API (see https://wiki.mozilla.org/NPAPI:ClearSiteData%5B1%5D), so I'm using
that
> term in all classes that are essentially a wrapper around that API. However,
> "site" is a somewhat fuzzy term, and the spec says that it actually means
domain
> (and Flash also keys LSOs on the domain), so I'm using "domain" in the cookie
> tree model where the semantic actually matters.
> 
> [1] I'm not using the NPAPI version, but I tried to keep the Pepper version as
> similar as possible.

I see. Thanks for the explanation :)

[commit-bot: I haz the power, 2012-08-01 16:07:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/10790150/19004

[commit-bot: I haz the power, 2012-08-01 17:33:54]

Change committed as 149439