Update our use of the NPAPI ClearSiteData API.

Update our use of the NPAPI ClearSiteData API.

BUG=58235
TEST=none

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

[Bernhard Bauer, 2011-01-10 17:41:52]

please review.

[stuartmorgan, 2011-01-10 17:53:45]

Drive-by:

http://codereview.chromium.org/6148006/diff/3001/chrome/browser/plugin_data_r...
File chrome/browser/plugin_data_remover.cc (left):

http://codereview.chromium.org/6148006/diff/3001/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.cc:23: // implementing the API.
This approach isn't going to scale well with other plugins supporting this.
Can't we have an API on the plugin object that checks support by looking at the
NPP_* function table struct, instead of hard-coding every plugin and version?

[Bernhard Bauer, 2011-01-10 17:57:31]

On 2011/01/10 17:53:45, stuartmorgan wrote:
> Drive-by:
> 
>
http://codereview.chromium.org/6148006/diff/3001/chrome/browser/plugin_data_r...
> File chrome/browser/plugin_data_remover.cc (left):
> 
>
http://codereview.chromium.org/6148006/diff/3001/chrome/browser/plugin_data_r...
> chrome/browser/plugin_data_remover.cc:23: // implementing the API.
> This approach isn't going to scale well with other plugins supporting this.
> Can't we have an API on the plugin object that checks support by looking at
the
> NPP_* function table struct, instead of hard-coding every plugin and version?

The problem is that we need to reflect this in the UI before we actually load
the plugin.

From what I've gathered, the number of plug-ins implementing this in the
foreseeable future will be rather small.

[jam, 2011-01-10 18:09:36]

lgtm

http://codereview.chromium.org/6148006/diff/3001/chrome/browser/plugin_data_r...
File chrome/browser/plugin_data_remover.cc (left):

http://codereview.chromium.org/6148006/diff/3001/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.cc:23: // implementing the API.
On 2011/01/10 17:53:45, stuartmorgan wrote:
> This approach isn't going to scale well with other plugins supporting this.
> Can't we have an API on the plugin object that checks support by looking at
the
> NPP_* function table struct, instead of hard-coding every plugin and version?

As Bernhard said, the number of plugins implementing this API will likely be 1
for a looooong time..

http://codereview.chromium.org/6148006/diff/3001/chrome/browser/plugin_data_r...
File chrome/browser/plugin_data_remover.cc (right):

http://codereview.chromium.org/6148006/diff/3001/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.cc:154: GURL(), kFlashMimeType,
allow_wildcard, &plugin, &mime_type)) {
nit: i think you need 4 spaces before GURL()

http://codereview.chromium.org/6148006/diff/3001/webkit/plugins/npapi/plugin_...
File webkit/plugins/npapi/plugin_lib.cc (right):

http://codereview.chromium.org/6148006/diff/3001/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_lib.cc:144: if (plugin_funcs_.clearsitedata != 0) {
nit: the "!= 0" and the brace brackets aren't necessary, here and below

http://codereview.chromium.org/6148006/diff/3001/webkit/plugins/npapi/plugin_...
File webkit/plugins/npapi/plugin_lib.h (right):

http://codereview.chromium.org/6148006/diff/3001/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_lib.h:76: // NPAPI method to clear locally stored
data (LSO's or "Flash cookies").
nit: since this is an NPAPI function, probably shouldn't make the comment
specific to Flash?

[stuartmorgan, 2011-01-10 18:11:11]

On 2011/01/10 17:57:31, Bernhard Bauer wrote:
> From what I've gathered, the number of plug-ins implementing this in the
> foreseeable future will be rather small.

Post-10, could we have each plugin report support back to the browser process
when it is initialized, and we could keep a persistent cache of
plugin+version->support, then when building the UI check the plugin and version
against that list (treating the stored version as a minimum version)? Then it
would work if the user has ever loaded that plugin, which is probably a safe bet
if they are trying to clear information it has stored.

[Bernhard Bauer, 2011-01-11 12:43:34]

On 2011/01/10 18:11:11, stuartmorgan wrote:
> On 2011/01/10 17:57:31, Bernhard Bauer wrote:
> > From what I've gathered, the number of plug-ins implementing this in the
> > foreseeable future will be rather small.
> 
> Post-10, could we have each plugin report support back to the browser process
> when it is initialized, and we could keep a persistent cache of
> plugin+version->support, then when building the UI check the plugin and
version
> against that list (treating the stored version as a minimum version)? Then it
> would work if the user has ever loaded that plugin, which is probably a safe
bet
> if they are trying to clear information it has stored.

Nice! Although with a clean profile we'd still have to start each plugin at
least once to find out whether it supports deleting data or not.

http://codereview.chromium.org/6148006/diff/3001/chrome/browser/plugin_data_r...
File chrome/browser/plugin_data_remover.cc (right):

http://codereview.chromium.org/6148006/diff/3001/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.cc:154: GURL(), kFlashMimeType,
allow_wildcard, &plugin, &mime_type)) {
On 2011/01/10 18:09:36, John Abd-El-Malek wrote:
> nit: i think you need 4 spaces before GURL()

Right, done.

http://codereview.chromium.org/6148006/diff/3001/webkit/plugins/npapi/plugin_...
File webkit/plugins/npapi/plugin_lib.cc (right):

http://codereview.chromium.org/6148006/diff/3001/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_lib.cc:144: if (plugin_funcs_.clearsitedata != 0) {
On 2011/01/10 18:09:36, John Abd-El-Malek wrote:
> nit: the "!= 0" and the brace brackets aren't necessary, here and below

Done.

http://codereview.chromium.org/6148006/diff/3001/webkit/plugins/npapi/plugin_...
File webkit/plugins/npapi/plugin_lib.h (right):

http://codereview.chromium.org/6148006/diff/3001/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_lib.h:76: // NPAPI method to clear locally stored
data (LSO's or "Flash cookies").
On 2011/01/10 18:09:36, John Abd-El-Malek wrote:
> nit: since this is an NPAPI function, probably shouldn't make the comment
> specific to Flash?

Hence the quotation marks around it. I wanted to use the more widely known
expression, even if it's slightly incorrect.

Note that I'm committing this version without changing the comment; if you feel
strongly about it, I'll do a followup commit.