Fix for broken "Load unpacked extension" popup

Fix for broken "Load unpacked extension" popup

In crrev.com/2794803003 I introduced a bug that caused "Load unpacked extension" screen (available under chrome://extensions) to not display any files. This CL fixes it.

BUG=707864
Review-Url: https://codereview.chromium.org/2840043002
Cr-Commit-Position: refs/heads/master@{#467631}
Committed: https://chromium.googlesource.com/chromium/src/+/55180f5ed3cd7386ef97e044d4084e5fb9afc67a

[Ivan Šandrk, 2017-04-25 18:49:06]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 18:49:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2017-04-25 18:51:59]

isandrk@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Ivan Šandrk, 2017-04-25 18:51:59]

Hey Devlin, apparently I wasn't careful enough when writing the original code.
Ptal!

https://codereview.chromium.org/2840043002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc
(right):

https://codereview.chromium.org/2840043002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc:38:
api_permission_set, (*granted_permissions)->manifest_permissions(),
Should I maybe have a temporary helper variable to shorten the following?:

(*granted_permissions)->...

[Devlin, 2017-04-25 18:57:48]

Whoops!  Good catch.

Can you add a unittest for this case?

https://codereview.chromium.org/2840043002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc
(right):

https://codereview.chromium.org/2840043002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc:38:
api_permission_set, (*granted_permissions)->manifest_permissions(),
On 2017/04/25 18:51:59, Ivan Šandrk wrote:
> Should I maybe have a temporary helper variable to shorten the following?:
> 
> (*granted_permissions)->...

I'd say it's probably not worth it, since this is resetting the granted
permissions, so a variable set to the old value would become unsafe very soon. 
But, if you wanted to, you could scope it, e.g.
api_permission_set.erase(APIPermission::kClipboardRead);
{
  const PermissionSet& granted = *granted_permissions;
  granted_permissions = base::MakeUnique<PermissionSet>(
      api_permission_set, granted.manifest_permissions(),
      granted.explicit_hosts(), granted.scriptable_hosts()));
}

which would discourage its unsafe use and would be okay.

[commit-bot: I haz the power, 2017-04-25 19:19:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 19:19:19]

Dry run: This issue passed the CQ dry run.

[Ivan Šandrk, 2017-04-25 19:42:11]

Can I submit the test in another CL? Just to get this fixed asap.

And regarding the test, you mean that I should update the corresponding unittest
for this file?

Also I'm unsure if the test would have any real value as this was an oversight
when writing the code - I'm thinking that I could've made a mistake in any place
of code and by that logic should test absolutely everything, but maybe I'm
missing something. Bottom line - if you want the test I'll write it :-)

https://codereview.chromium.org/2840043002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc
(right):

https://codereview.chromium.org/2840043002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc:38:
api_permission_set, (*granted_permissions)->manifest_permissions(),
On 2017/04/25 18:57:48, Devlin wrote:
> On 2017/04/25 18:51:59, Ivan Šandrk wrote:
> > Should I maybe have a temporary helper variable to shorten the following?:
> > 
> > (*granted_permissions)->...
> 
> I'd say it's probably not worth it, since this is resetting the granted
> permissions, so a variable set to the old value would become unsafe very soon.

> But, if you wanted to, you could scope it, e.g.
> api_permission_set.erase(APIPermission::kClipboardRead);
> {
>   const PermissionSet& granted = *granted_permissions;
>   granted_permissions = base::MakeUnique<PermissionSet>(
>       api_permission_set, granted.manifest_permissions(),
>       granted.explicit_hosts(), granted.scriptable_hosts()));
> }
> 
> which would discourage its unsafe use and would be okay.

I guess it's not worth the bother.

[Devlin, 2017-04-26 00:55:02]

On 2017/04/25 19:42:11, Ivan Šandrk wrote:
> Can I submit the test in another CL? Just to get this fixed asap.

I think the test should be fairly easy to write, so I'd prefer it go in with
this patch (just to make sure we remember to do it :)).  If it turns out to be
more complicated, lemme know, and we can revisit.

> And regarding the test, you mean that I should update the corresponding
unittest
> for this file?

Yep

> Also I'm unsure if the test would have any real value as this was an oversight
> when writing the code - I'm thinking that I could've made a mistake in any
place
> of code and by that logic should test absolutely everything, but maybe I'm
> missing something. Bottom line - if you want the test I'll write it :-)

It can be ambiguous when to add a test, I agree.  A good rule of thumb is that
we should test *behavior*, and not (always) *code*.  I think, in this case, this
falls into a behavioral test.  The behavior of the CrOS delegate should be to
filter out certain permissions, while not removing others.  I think it's
important enough to warrant a test case, and the fact that we both missed it the
first time around means it's not entirely obvious. :)  Just like we'd want to
test other cases that can be handled by a single line of code (e.g. off-by-one
errors), I think it's important enough to test here.

And, of course, there's the fact that the test itself should be pretty
straightforward.  If this required an interactive ui test or similar (where the
test maintenance might outweight the test benefit), I might feel differently,
but I think this shouldn't be too bad.

[Ivan Šandrk, 2017-04-26 13:14:33]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 13:14:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2017-04-26 13:25:02]

On 2017/04/26 00:55:02, Devlin wrote:
> On 2017/04/25 19:42:11, Ivan Šandrk wrote:
> > Can I submit the test in another CL? Just to get this fixed asap.
> 
> I think the test should be fairly easy to write, so I'd prefer it go in with
> this patch (just to make sure we remember to do it :)).  If it turns out to be
> more complicated, lemme know, and we can revisit.

Yeah, just my tendency to rush things to get fixed immediately, I guess there's
no rush on something like this.

> > Also I'm unsure if the test would have any real value as this was an
oversight
> > when writing the code - I'm thinking that I could've made a mistake in any
> place
> > of code and by that logic should test absolutely everything, but maybe I'm
> > missing something. Bottom line - if you want the test I'll write it :-)
> 
> It can be ambiguous when to add a test, I agree.  A good rule of thumb is that
> we should test *behavior*, and not (always) *code*.  I think, in this case,
this
> falls into a behavioral test.  The behavior of the CrOS delegate should be to
> filter out certain permissions, while not removing others.  I think it's
> important enough to warrant a test case, and the fact that we both missed it
the
> first time around means it's not entirely obvious. :)  Just like we'd want to
> test other cases that can be handled by a single line of code (e.g. off-by-one
> errors), I think it's important enough to test here.

Thanks for explaining, makes sense.

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc
(right):

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc:37:
*granted_permissions = PermissionSet::CreateDifference(
Does this approach seem better? Maybe a bit less prone to missing something, as
we need to explicitly remove what we don't want.

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc:32:
class MockManifestPermission : public ManifestPermission {
I've c/p-ed this from manifest_permission_set_unittest. Maybe I should extract
it from there into mock_manifest_permission.cc/h so it's easier to reuse?

[commit-bot: I haz the power, 2017-04-26 14:19:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 14:19:14]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-04-26 14:58:26]

lgtm; thanks for the test!

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc
(right):

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos.cc:37:
*granted_permissions = PermissionSet::CreateDifference(
On 2017/04/26 13:25:02, Ivan Šandrk wrote:
> Does this approach seem better? Maybe a bit less prone to missing something,
as
> we need to explicitly remove what we don't want.

yep, this looks better.  Good idea!

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc:32:
class MockManifestPermission : public ManifestPermission {
On 2017/04/26 13:25:02, Ivan Šandrk wrote:
> I've c/p-ed this from manifest_permission_set_unittest. Maybe I should extract
> it from there into mock_manifest_permission.cc/h so it's easier to reuse?

That would work.  Another option would be to just use a "real" manifest
permission, e.g. SocketsManifestPermission.  I'm fine with either.

(And if you go with the extraction route, feel free to defer that to a separate
CL if you add a TODO)

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc:149:
auto expected_permissions = PermissionSet::CreateDifference(
nitty nit: This mirrors the code a bit too much. :)  I think this would be
cleaner when we construct each set individually and omit clipboard in one of
them, e.g. (pseudo-code):

PermissionSet original = {clipboard, other1, other2};
PermissionSet expected = {other1, other2};

so that it's clear what the difference is.

Given creating the set is more lines of code than we'd want to dupe, I'd suggest
having CreatePermissions take a bool include_clipboard, so that then we do:
PermissionSet original = CreatePermissions(true);
...
EXPECT_EQ(CreatePermissions(false), *granted);

WDYT?

(I realize that fundamentally it's the same, but IMHO this makes it a bit more
clear what exactly the expected difference is)

[Ivan Šandrk, 2017-04-26 17:02:22]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 17:02:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2017-04-26 17:04:44]

Just one more qq about using a default parameter.

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc:32:
class MockManifestPermission : public ManifestPermission {
On 2017/04/26 14:58:26, Devlin wrote:
> On 2017/04/26 13:25:02, Ivan Šandrk wrote:
> > I've c/p-ed this from manifest_permission_set_unittest. Maybe I should
extract
> > it from there into mock_manifest_permission.cc/h so it's easier to reuse?
> 
> That would work.  Another option would be to just use a "real" manifest
> permission, e.g. SocketsManifestPermission.  I'm fine with either.
> 
> (And if you go with the extraction route, feel free to defer that to a
separate
> CL if you add a TODO)

TODO it is.

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc:149:
auto expected_permissions = PermissionSet::CreateDifference(
On 2017/04/26 14:58:26, Devlin wrote:
> nitty nit: This mirrors the code a bit too much. :)  I think this would be
> cleaner when we construct each set individually and omit clipboard in one of
> them, e.g. (pseudo-code):
> 
> PermissionSet original = {clipboard, other1, other2};
> PermissionSet expected = {other1, other2};
> 
> so that it's clear what the difference is.
> 
> Given creating the set is more lines of code than we'd want to dupe, I'd
suggest
> having CreatePermissions take a bool include_clipboard, so that then we do:
> PermissionSet original = CreatePermissions(true);
> ...
> EXPECT_EQ(CreatePermissions(false), *granted);
> 
> WDYT?
> 
> (I realize that fundamentally it's the same, but IMHO this makes it a bit more
> clear what exactly the expected difference is)

Good point about the mirroring, I actually had a weird feeling about it.

Good suggestion, done.

I'm using a default parameter on CreatePermissions, do you think that's ok or
maybe bad for some reason?

[commit-bot: I haz the power, 2017-04-26 17:37:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 17:37:05]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-04-26 18:34:52]

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2840043002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc:149:
auto expected_permissions = PermissionSet::CreateDifference(
On 2017/04/26 17:04:44, Ivan Šandrk wrote:
> On 2017/04/26 14:58:26, Devlin wrote:
> > nitty nit: This mirrors the code a bit too much. :)  I think this would be
> > cleaner when we construct each set individually and omit clipboard in one of
> > them, e.g. (pseudo-code):
> > 
> > PermissionSet original = {clipboard, other1, other2};
> > PermissionSet expected = {other1, other2};
> > 
> > so that it's clear what the difference is.
> > 
> > Given creating the set is more lines of code than we'd want to dupe, I'd
> suggest
> > having CreatePermissions take a bool include_clipboard, so that then we do:
> > PermissionSet original = CreatePermissions(true);
> > ...
> > EXPECT_EQ(CreatePermissions(false), *granted);
> > 
> > WDYT?
> > 
> > (I realize that fundamentally it's the same, but IMHO this makes it a bit
more
> > clear what exactly the expected difference is)
> 
> Good point about the mirroring, I actually had a weird feeling about it.
> 
> Good suggestion, done.
> 
> I'm using a default parameter on CreatePermissions, do you think that's ok or
> maybe bad for some reason?

We *used* to have strong rules about not allowing default values, but they've
since been lifted and these are now allowed, but with scrutiny.  Given this
function is local to this file, this would be fine.

My personal preference would be to require the parameter, since it's used in
relatively few places, but that's entirely subjective, so feel free to do
whichever you prefer. :)

[Ivan Šandrk, 2017-04-27 10:15:40]

The CQ bit was checked by isandrk@chromium.org

[Ivan Šandrk, 2017-04-27 10:15:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2840043002/#ps40001
(title: "TODO, CreatePermissions with a parameter")

[commit-bot: I haz the power, 2017-04-27 10:15:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 10:20:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493288140428840,
"parent_rev": "892740a77a628129df2c93c96dbc3cf448b170b1", "commit_rev":
"55180f5ed3cd7386ef97e044d4084e5fb9afc67a"}

[commit-bot: I haz the power, 2017-04-27 10:20:50]

Description was changed from

==========
Fix for broken "Load unpacked extension" popup

In crrev.com/2794803003 I introduced a bug that caused "Load unpacked extension"
screen (available under chrome://extensions) to not display any files. This CL
fixes it.

BUG=707864
==========

to

==========
Fix for broken "Load unpacked extension" popup

In crrev.com/2794803003 I introduced a bug that caused "Load unpacked extension"
screen (available under chrome://extensions) to not display any files. This CL
fixes it.

BUG=707864

Review-Url: https://codereview.chromium.org/2840043002
Cr-Commit-Position: refs/heads/master@{#467631}
Committed:
https://chromium.googlesource.com/chromium/src/+/55180f5ed3cd7386ef97e044d408...
==========

[commit-bot: I haz the power, 2017-04-27 10:20:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/55180f5ed3cd7386ef97e044d408...