Extensions: Don't pass the selected unpacked extension path from JS to C++

Extensions: Don't pass the selected unpacked extension path from JS to C++
handler.

This is an unecessary hoop to jump through since the C++ is the original
callback of the file dialog.

BUG=117715
TEST=none
TBR=sky
R=csilv,aa


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

[James Hawkins, 2012-03-14 22:39:34]

+cc: jschuh, abarth, cevans

[Aaron Boodman, 2012-03-14 23:12:44]

LGTM, but would be easier to review if you made the cleanup change separately.

http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/extension_settings_handler.h (right):

http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/extension_settings_handler.h:179: // once to the
page, otherwise we'd be calling JavaScript functions on objects
yay for properly capitalizing this word :)

[Evan Stade, 2012-03-14 23:13:45]

http://codereview.chromium.org/9703039/diff/1/chrome/browser/resources/extens...
File chrome/browser/resources/extensions/extensions.js (right):

http://codereview.chromium.org/9703039/diff/1/chrome/browser/resources/extens...
chrome/browser/resources/extensions/extensions.js:77: // TODO(jhawkins):
Refactor metrics support out of options and use it
MetricsHandler

http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (left):

http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/extension_settings_handler.cc:152:
web_ui()->RegisterMessageCallback("extensionSettingsSelectFilePath",
this is still used in the pack extension overlay

http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/extension_settings_handler.cc:624: void
ExtensionSettingsHandler::HandleAutoUpdateMessage(const ListValue* args) {
moving all this code around makes it difficult to see the relevant changes

[James Hawkins, 2012-03-14 23:15:06]

On Wed, Mar 14, 2012 at 4:12 PM, <aa@chromium.org> wrote:

> LGTM, but would be easier to review if you made the cleanup change
> separately.
>
>
Sorry about that.  For the merge to M18, I'll likely (have to) do that by
hand and have you guys re-review.  Wanted to land this change first though.


>
> http://codereview.chromium.**org/9703039/diff/1/chrome/**
>
browser/ui/webui/options/**extension_settings_handler.h<http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options/extension_settings_handler.h>
> File chrome/browser/ui/webui/**options/extension_settings_**handler.h
> (right):
>
> http://codereview.chromium.**org/9703039/diff/1/chrome/**
>
browser/ui/webui/options/**extension_settings_handler.h#**newcode179<http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options/extension_settings_handler.h#newcode179>
> chrome/browser/ui/webui/**options/extension_settings_**handler.h:179: //
> once to the page, otherwise we'd be calling JavaScript functions on
> objects
> yay for properly capitalizing this word :)
>
>
http://codereview.chromium.**org/9703039/<http://codereview.chromium.org/9703...
>

[csilv, 2012-03-14 23:20:19]

lgtm

http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/extension_settings_handler.cc:635: int
file_type_index = 0;
You can remove file_type_index, it isn't being used anymore.

[James Hawkins, 2012-03-15 00:24:13]

http://codereview.chromium.org/9703039/diff/1/chrome/browser/resources/extens...
File chrome/browser/resources/extensions/extensions.js (right):

http://codereview.chromium.org/9703039/diff/1/chrome/browser/resources/extens...
chrome/browser/resources/extensions/extensions.js:77: // TODO(jhawkins):
Refactor metrics support out of options and use it
On 2012/03/14 23:13:45, Evan Stade wrote:
> MetricsHandler

I meant pull it out of pref_ui.js so we can add metric="blah" to buttons
declaratively.

http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (left):

http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/extension_settings_handler.cc:152:
web_ui()->RegisterMessageCallback("extensionSettingsSelectFilePath",
On 2012/03/14 23:13:45, Evan Stade wrote:
> this is still used in the pack extension overlay

Done.

http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/9703039/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/extension_settings_handler.cc:635: int
file_type_index = 0;
On 2012/03/14 23:20:19, csilv wrote:
> You can remove file_type_index, it isn't being used anymore.

Done.

[James Hawkins, 2012-03-15 16:28:07]

estade: ping

[James Hawkins, 2012-03-15 22:18:38]

estade: I'm checking in, will reply to any further review comments in a
follow-up CL.

[commit-bot: I haz the power, 2012-03-20 18:38:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jhawkins@chromium.org/9703039/4003

[commit-bot: I haz the power, 2012-03-20 18:38:52]

Can't apply patch for file
chrome/browser/ui/webui/options/extension_settings_handler.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/webui/options/extension_settings_handler.cc
Hunk #5 FAILED at 554.
Hunk #6 succeeded at 869 (offset -3 lines).
Hunk #7 succeeded at 950 (offset -3 lines).
1 out of 7 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/options/extension_settings_handler.cc.rej

[Evan Stade, 2012-03-20 19:56:07]

lgtm

http://codereview.chromium.org/9703039/diff/4003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/pack_extension_handler.cc (right):

http://codereview.chromium.org/9703039/diff/4003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/pack_extension_handler.cc:115:
!args->GetString(1, &private_key_path_))
curlies

[James Hawkins, 2012-03-23 21:49:38]

http://codereview.chromium.org/9703039/diff/4003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/pack_extension_handler.cc (right):

http://codereview.chromium.org/9703039/diff/4003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/pack_extension_handler.cc:115:
!args->GetString(1, &private_key_path_))
On 2012/03/20 19:56:07, Evan Stade wrote:
> curlies

The C++ style guide does not require braces for multi-line logic blocks (or I
can't find it).

[Evan Stade, 2012-03-23 21:54:09]

http://codereview.chromium.org/9703039/diff/4003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/pack_extension_handler.cc (right):

http://codereview.chromium.org/9703039/diff/4003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/pack_extension_handler.cc:115:
!args->GetString(1, &private_key_path_))
On 2012/03/23 21:49:38, James Hawkins wrote:
> On 2012/03/20 19:56:07, Evan Stade wrote:
> > curlies
> 
> The C++ style guide does not require braces for multi-line logic blocks (or I
> can't find it).

chrome convention, supported by style guide: "conditional or loop statements
with complex conditions or statements may be more readable with curly braces"

[James Hawkins, 2012-03-23 22:04:33]

http://codereview.chromium.org/9703039/diff/4003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/pack_extension_handler.cc (right):

http://codereview.chromium.org/9703039/diff/4003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/pack_extension_handler.cc:115:
!args->GetString(1, &private_key_path_))
On 2012/03/23 21:54:09, Evan Stade wrote:
> On 2012/03/23 21:49:38, James Hawkins wrote:
> > On 2012/03/20 19:56:07, Evan Stade wrote:
> > > curlies
> > 
> > The C++ style guide does not require braces for multi-line logic blocks (or
I
> > can't find it).
> 
> chrome convention, supported by style guide: "conditional or loop statements
> with complex conditions or statements may be more readable with curly braces"

OK, then it's an 'Optional nit'.  I've seen plenty of code to the contrary to
suggest it's not a project-wide convention.

[commit-bot: I haz the power, 2012-03-24 23:55:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jhawkins@chromium.org/9703039/12001

[commit-bot: I haz the power, 2012-03-24 23:55:17]

Presubmit check for 9703039-12001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui

Presubmit checks took 1.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-03-25 00:12:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jhawkins@chromium.org/9703039/12001

[commit-bot: I haz the power, 2012-03-25 01:49:04]

Change committed as 128801