Don't use command line flags for enabling the internal pdf plugin. Instead, ...

Don't use command line flags for enabling the internal pdf plugin.  Instead, disable it by default using the same mechanism that we use for about:plugins.  Once we enable it by default, we just need to flip a boolean in the code and it'll be enabled for everyone.


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

[viettrungluu, 2010-05-21 20:29:41]

Aargh. Been continually busy so far today, so I guess I'll send you my comments
so far.

I want to stare at it some more to make sure it'll do the right thing later when
we turn it on by default....

http://codereview.chromium.org/2080016/diff/10001/6003
File chrome/browser/plugin_service.cc (right):

http://codereview.chromium.org/2080016/diff/10001/6003#newcode65
chrome/browser/plugin_service.cc:65: update_internal_dir = (cur_internal_dir !=
last_internal_dir);
(See below.)

http://codereview.chromium.org/2080016/diff/10001/6003#newcode110
chrome/browser/plugin_service.cc:110: plugin->SetString(L"path",
plugin_path.value());
Since you actually update the plugin path in prefs here, you'll also want to
update the last internal directory in prefs as well.

http://codereview.chromium.org/2080016/diff/10001/6003#newcode120
chrome/browser/plugin_service.cc:120: // overriden the default.
s/overriden/overridden/

[viettrungluu, 2010-05-21 21:59:52]

LGTM.

http://codereview.chromium.org/2080016/diff/10001/6003
File chrome/browser/plugin_service.cc (right):

http://codereview.chromium.org/2080016/diff/10001/6003#newcode72
chrome/browser/plugin_service.cc:72: if (enable_internal_pdf_ &&
Could you just add a comment explaining what this is for?

http://codereview.chromium.org/2080016/diff/10001/6007
File chrome/common/chrome_switches.cc (left):

http://codereview.chromium.org/2080016/diff/10001/6007#oldcode500
chrome/common/chrome_switches.cc:500: const char kInternalPDF[]                 
= "internal-pdf";
nit: one more space before the '=' :)

[jam, 2010-05-21 22:40:10]

http://codereview.chromium.org/2080016/diff/10001/6003
File chrome/browser/plugin_service.cc (right):

http://codereview.chromium.org/2080016/diff/10001/6003#newcode72
chrome/browser/plugin_service.cc:72: if (enable_internal_pdf_ &&
On 2010/05/21 21:59:54, viettrungluu wrote:
> Could you just add a comment explaining what this is for?

Done.

http://codereview.chromium.org/2080016/diff/10001/6003#newcode110
chrome/browser/plugin_service.cc:110: plugin->SetString(L"path",
plugin_path.value());
On 2010/05/21 20:29:41, viettrungluu wrote:
> Since you actually update the plugin path in prefs here, you'll also want to
> update the last internal directory in prefs as well.

Done.

http://codereview.chromium.org/2080016/diff/10001/6003#newcode120
chrome/browser/plugin_service.cc:120: // overriden the default.
On 2010/05/21 20:29:41, viettrungluu wrote:
> s/overriden/overridden/

Done.

http://codereview.chromium.org/2080016/diff/10001/6007
File chrome/common/chrome_switches.cc (left):

http://codereview.chromium.org/2080016/diff/10001/6007#oldcode500
chrome/common/chrome_switches.cc:500: const char kInternalPDF[]                 
= "internal-pdf";
On 2010/05/21 21:59:54, viettrungluu wrote:
> nit: one more space before the '=' :)

this is being deleted

[viettrungluu, 2010-05-21 22:42:06]

http://codereview.chromium.org/2080016/diff/10001/6007
File chrome/common/chrome_switches.cc (left):

http://codereview.chromium.org/2080016/diff/10001/6007#oldcode500
chrome/common/chrome_switches.cc:500: const char kInternalPDF[]                 
= "internal-pdf";
On 2010/05/21 22:40:10, John Abd-El-Malek wrote:
> On 2010/05/21 21:59:54, viettrungluu wrote:
> > nit: one more space before the '=' :)
> 
> this is being deleted

Don't I feel dumb now?