[Extensions] Make a chrome.developerPrivate.getExtensionsInfo function

[Extensions] Make a chrome.developerPrivate.getExtensionsInfo function

Make a new function, developerPrivate.getExtensionsInfo,
to handle generating and returning info about installed
extensions. We do this, instead of adjusting the current
(developerPrivate.getItemsInfo) to allow for breaking
changes - which lets us have consistent style and fewer
parameters.

BUG=461039
Committed: https://crrev.com/609f7a872624668960c7da22221924c47a8f885f
Cr-Commit-Position: refs/heads/master@{#320355}

[Devlin, 2015-03-07 03:15:56]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-03-07 03:21:02]

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

[Devlin, 2015-03-07 03:21:03]

Heya Ben - this one's a right PITA.  So it's still a little rough, but I'd like
your feedback on it - in particular, the IDL changes.  As we discussed, this
basically replaces the getItemsInfo with a getExtensionsInfo, so that we can
have a result that is:
a) properly formatted
b) (hopefully) more logically named
c) up-to-date

It also adds a bunch of enums and a few other dictionaries to kinda clear it up
a bit.

Plan for getting these through is to:
a) Submit this patch (after some polish)
b) Update webui to use it, and clean up extension settings handler of all that
generation (woohoo!)
c) Update webui to get the real benefits - observe changes to a particular
extension, and request only that info, etc.
d [maybe]) update ADT, Spark to use the new api calls and get rid of the
deprecated ones.

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:129: boolean
actionButtonHidden;  // enable_show_button
The chickenscratch comments map to the names in extension_list.js.

[Devlin, 2015-03-07 06:16:44]

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:45: enum RunStateValue {
On second thought, I may change this into a
dictionary AccessModifier {
  boolean isEnabled;
  boolean isActive;
};

(name pending)

Mostly because in some cases, the idea that a feature is active/enabled could be
different.  For instance, in the case of script injection.  Could probably work
either way, but the dictionary might be clearer...

[not at google - send to devlin, 2015-03-09 18:44:01]

Wow that is a big change. I commented a bit on the IDL. I like the structure
with the main API then the "mangle" version, and pulling the non-data-shuffling
code into their own classes. Even if that's only 1 class.

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:45: enum RunStateValue {
On 2015/03/07 06:16:43, Devlin wrote:
> On second thought, I may change this into a
> dictionary AccessModifier {
>   boolean isEnabled;
>   boolean isActive;
> };
> 
> (name pending)
> 
> Mostly because in some cases, the idea that a feature is active/enabled could
be
> different.  For instance, in the case of script injection.  Could probably
work
> either way, but the dictionary might be clearer...

Yes this enum doesn't make sense to me.

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:54: UNKNOWN
there is also local CRX's (like drag-drop into chrome://extensions)

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:78: DOMString function;
You might have a problem with calling something "function", it's a reserved word
in JS (yes you could put it in a string, but that's awkward).

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:91: dictionary RuntimeError {
This shares a lot with ManifestError, you could have a separate dictionary type
like, err, "Error"?

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:133: DisableReasons
disableReasons;  //suspicious, corrupt, updaterequired
space after //

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:143: Location location;  //
is_unpacked, is from store, allow reload
is from store --> is_from_store --> etc.

[Devlin, 2015-03-09 19:28:21]

Just comments on comments - no code change. :)

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:54: UNKNOWN
On 2015/03/09 18:44:01, kalman wrote:
> there is also local CRX's (like drag-drop into chrome://extensions)

Hmm... good point.  Though if we match this up with "locationText", that is
lumped into "UNKNOWN" (and I don't think we really say "installed from chrome
extensions page" anywhere).  Thoughts?

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:78: DOMString function;
On 2015/03/09 18:44:01, kalman wrote:
> You might have a problem with calling something "function", it's a reserved
word
> in JS (yes you could put it in a string, but that's awkward).

There's no problem having a "function" property on an object.

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:91: dictionary RuntimeError {
On 2015/03/09 18:44:01, kalman wrote:
> This shares a lot with ManifestError, you could have a separate dictionary
type
> like, err, "Error"?

But (our) IDL doesn't have inheritance, and, of course, it's kind of annoying to
have an object with a bunch of things that are *always* null in some cases, and
*always* not in others - it limits the usefulness of the closurecompiler and
type-ing comments.  Unless you meant using encapsulation, and having something
like:
RuntimeError {
  Error error;
  <runtime-error-specific>
}?

Which works, but is ever-so-slightly annoying to work with (in js,
error.error.message is ugly, compared to error.message).

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:133: DisableReasons
disableReasons;  //suspicious, corrupt, updaterequired
On 2015/03/09 18:44:01, kalman wrote:
> space after //

Chickenscratch will be removed. :P  Only left it in if you wanted a mapping to
extension_list.js's version for reference.  I don't plan on landing it with it.

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:143: Location location;  //
is_unpacked, is from store, allow reload
On 2015/03/09 18:44:01, kalman wrote:
> is from store --> is_from_store --> etc.

Ditto.

[not at google - send to devlin, 2015-03-09 21:11:39]

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/989813002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:91: dictionary RuntimeError {
On 2015/03/09 19:28:20, Devlin wrote:
> On 2015/03/09 18:44:01, kalman wrote:
> > This shares a lot with ManifestError, you could have a separate dictionary
> type
> > like, err, "Error"?
> 
> But (our) IDL doesn't have inheritance, and, of course, it's kind of annoying
to
> have an object with a bunch of things that are *always* null in some cases,
and
> *always* not in others - it limits the usefulness of the closurecompiler and
> type-ing comments.  Unless you meant using encapsulation, and having something
> like:
> RuntimeError {
>   Error error;
>   <runtime-error-specific>
> }?
> 
> Which works, but is ever-so-slightly annoying to work with (in js,
> error.error.message is ugly, compared to error.message).

I mean:

dictionary ErrorBase {
  ErrorType type;
  DOMString extensionId;
  boolean fromIncognito;
  DOMString source;
  DOMString message;
};

dictionary ManifestError {
  ErrorBase base;
  DOMString manifestKey;
  DOMString? manifestSpecific;
};

dictionary RuntimeError {
  ErrorBase base;
  long occurrences;
  long renderViewId;
  long renderProcessId;
  boolean canInspect;
  StackFrame[] stackFrame;
};

[Devlin, 2015-03-09 22:07:15]

Okay, I think it's ready for a real review.

[not at google - send to devlin, 2015-03-11 17:43:05]

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:349: //
TODO(devlin): This won't work with apps (CORS).
We should convert to data URLs.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:795:
ExtensionIconSet::MATCH_BIGGER);
All of this data population is... incredibly hard to review, and not 100% test -
but if it were 100% tested then that's basically just duplicating all this logic
in a test.

I guess that if we know that the CDE still works - and the fact that the CDE is
based on the data mangling - then I have some confidence. And once you convert
chrome://extensions to use this API it'll become clear if there are any little
problems. I'm... happy.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.h:218: };
Was this lack of a blank line intentional?

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_mangle.cc
(right):

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_mangle.cc:3:
// found in the LICENSE file.
Have you tested that CDE still works?

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc
(right):

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:46:
// For live pages, use the full URL.
Why? Is this part of the API? Seems like the sort of thing that the user of the
API should figure out: give it a valid URL, and let it pull out the path if it
wants it.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:97:
-1,
Why are these -1?

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:115:
VIEW_TYPE_EXTENSION_POPUP == host_type ||
No yoda expressions; use host_type == VIEW_TYPE_EXTENSION_POPUP.
Also: parens around the body here, it's a 3-line condition.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:116:
VIEW_TYPE_EXTENSION_DIALOG == host_type)
Why are popups and dialogs filtered out anyway? That's odd.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:119:
GURL url = web_contents->GetURL();
const GURL&

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/inspectable_views_finder.h
(right):

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.h:30:
// Finds inspectable views for the extensions.
You might want to mention that this deals specifically with the structures
generated from developer_private.idl. It's not just an "inspectable views
finder", it's an "inspectable views finder that returns views as represented by
the JSON schema compiler"

(as opposed to RenderFrameHosts, which is what one might expect from a class
like this)

A comment will suffice.

https://codereview.chromium.org/989813002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/989813002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:49: // "Unknown" includes
crx's installed from chrome://extensions.
Why not make it known then (even if whoever uses this API doesn't care)?

https://codereview.chromium.org/989813002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:93: DOMString?
manifestSpecific;
What is this? I can't tell from just the name of the field; perhaps a field
rename would be enough.

[Devlin, 2015-03-11 21:45:54]

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:349: //
TODO(devlin): This won't work with apps (CORS).
On 2015/03/11 17:43:05, kalman wrote:
> We should convert to data URLs.

Yep, I know. Updated the TODO to be more specific.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:795:
ExtensionIconSet::MATCH_BIGGER);
On 2015/03/11 17:43:05, kalman wrote:
> All of this data population is... incredibly hard to review, and not 100% test
-
> but if it were 100% tested then that's basically just duplicating all this
logic
> in a test.
> 
> I guess that if we know that the CDE still works - and the fact that the CDE
is
> based on the data mangling - then I have some confidence. And once you convert
> chrome://extensions to use this API it'll become clear if there are any little
> problems. I'm... happy.

Yeah, I had the same dilemma when writing it.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.h:218: };
On 2015/03/11 17:43:05, kalman wrote:
> Was this lack of a blank line intentional?

Nope.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_mangle.cc
(right):

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_mangle.cc:3:
// found in the LICENSE file.
On 2015/03/11 17:43:05, kalman wrote:
> Have you tested that CDE still works?

Yes.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc
(right):

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:46:
// For live pages, use the full URL.
On 2015/03/11 17:43:05, kalman wrote:
> Why? Is this part of the API? Seems like the sort of thing that the user of
the
> API should figure out: give it a valid URL, and let it pull out the path if it
> wants it.

I'd be inclined to agree, but kinda wanna leave it this way for backwards
compatability (this is used by ExtensionSettingsHandler and getItemsInfo).

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:97:
-1,
On 2015/03/11 17:43:05, kalman wrote:
> Why are these -1?

For some very strange reason, background pages have -1 for rvh/rvp.  I'm
inclined to leave it that way for now for the same reason as above.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:115:
VIEW_TYPE_EXTENSION_POPUP == host_type ||
On 2015/03/11 17:43:05, kalman wrote:
> No yoda expressions; use host_type == VIEW_TYPE_EXTENSION_POPUP.
> Also: parens around the body here, it's a 3-line condition.

Like them either, I did not.  Trying to make a simple copy-paste, I was.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:116:
VIEW_TYPE_EXTENSION_DIALOG == host_type)
On 2015/03/11 17:43:05, kalman wrote:
> Why are popups and dialogs filtered out anyway? That's odd.

My guess is because they are inspected separately (e.g., right clicking the
extension icon).  Not sure if we *need* to do that, but we can always change it
later if appropriate.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:119:
GURL url = web_contents->GetURL();
On 2015/03/11 17:43:05, kalman wrote:
> const GURL&

Done.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/inspectable_views_finder.h
(right):

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.h:30:
// Finds inspectable views for the extensions.
On 2015/03/11 17:43:05, kalman wrote:
> You might want to mention that this deals specifically with the structures
> generated from developer_private.idl. It's not just an "inspectable views
> finder", it's an "inspectable views finder that returns views as represented
by
> the JSON schema compiler"
> 
> (as opposed to RenderFrameHosts, which is what one might expect from a class
> like this)
> 
> A comment will suffice.

Done. (I thought it might be obvious, since it was in api/developer_private, but
hey :))

https://codereview.chromium.org/989813002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/989813002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:49: // "Unknown" includes
crx's installed from chrome://extensions.
On 2015/03/11 17:43:05, kalman wrote:
> Why not make it known then (even if whoever uses this API doesn't care)?

We don't have an easy way of knowing.  We can always add it later, as needed.

https://codereview.chromium.org/989813002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:93: DOMString?
manifestSpecific;
On 2015/03/11 17:43:05, kalman wrote:
> What is this? I can't tell from just the name of the field; perhaps a field
> rename would be enough.

It's the "specific" part of the manifest key that went wrong. E.g., a particular
unrecognized permission.

https://codereview.chromium.org/989813002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:174: DOMString?
disabled_reason;
This is never set. Removing.

https://codereview.chromium.org/989813002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/developer_private.idl:177: boolean allow_activity;
For posterity, note that this is never, and has never been, set.

[not at google - send to devlin, 2015-03-11 21:50:24]

lgtm, 2 more comments

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc
(right):

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:46:
// For live pages, use the full URL.
On 2015/03/11 21:45:53, Devlin wrote:
> On 2015/03/11 17:43:05, kalman wrote:
> > Why? Is this part of the API? Seems like the sort of thing that the user of
> the
> > API should figure out: give it a valid URL, and let it pull out the path if
it
> > wants it.
> 
> I'd be inclined to agree, but kinda wanna leave it this way for backwards
> compatability (this is used by ExtensionSettingsHandler and getItemsInfo).

Backwards compatibility with a bad API? I think we'll be fine to change it. The
mangler can handle any mangling, and updating chrome://extensions shouldn't be a
drama.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:116:
VIEW_TYPE_EXTENSION_DIALOG == host_type)
On 2015/03/11 21:45:53, Devlin wrote:
> On 2015/03/11 17:43:05, kalman wrote:
> > Why are popups and dialogs filtered out anyway? That's odd.
> 
> My guess is because they are inspected separately (e.g., right clicking the
> extension icon).  Not sure if we *need* to do that, but we can always change
it
> later if appropriate.

Alright, well something needs to change, because the naming does not match up
with the behavior, and that's confusing. So either:
A- rename this method
B- don't filter out popups and dialogs, then do it on the JS side - see other
comment about supporting backwards compatible behavior in the mangler.
I think B is fine. A popup is modal, and I don't even know what an extension
dialog is.

[Devlin, 2015-03-11 23:34:07]

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc
(right):

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:46:
// For live pages, use the full URL.
On 2015/03/11 21:50:24, kalman wrote:
> On 2015/03/11 21:45:53, Devlin wrote:
> > On 2015/03/11 17:43:05, kalman wrote:
> > > Why? Is this part of the API? Seems like the sort of thing that the user
of
> > the
> > > API should figure out: give it a valid URL, and let it pull out the path
if
> it
> > > wants it.
> > 
> > I'd be inclined to agree, but kinda wanna leave it this way for backwards
> > compatability (this is used by ExtensionSettingsHandler and getItemsInfo).
> 
> Backwards compatibility with a bad API? I think we'll be fine to change it.
The
> mangler can handle any mangling, and updating chrome://extensions shouldn't be
a
> drama.

Done.

https://codereview.chromium.org/989813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:116:
VIEW_TYPE_EXTENSION_DIALOG == host_type)
On 2015/03/11 21:50:24, kalman wrote:
> On 2015/03/11 21:45:53, Devlin wrote:
> > On 2015/03/11 17:43:05, kalman wrote:
> > > Why are popups and dialogs filtered out anyway? That's odd.
> > 
> > My guess is because they are inspected separately (e.g., right clicking the
> > extension icon).  Not sure if we *need* to do that, but we can always change
> it
> > later if appropriate.
> 
> Alright, well something needs to change, because the naming does not match up
> with the behavior, and that's confusing. So either:
> A- rename this method
> B- don't filter out popups and dialogs, then do it on the JS side - see other
> comment about supporting backwards compatible behavior in the mangler.
> I think B is fine. A popup is modal, and I don't even know what an extension
> dialog is.

Done.

[Devlin, 2015-03-12 00:04:40]

rdevlin.cronin@chromium.org changed reviewers:
+ dbeam@chromium.org, mpearson@chromium.org

[Devlin, 2015-03-12 00:04:40]

+Dan for the closure_compiler and extension_list.js.
+Mark for the histograms.

[Dan Beam, 2015-03-12 00:23:15]

lgtm

https://codereview.chromium.org/989813002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/989813002/diff/100001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:731:
extension.views.forEach(function(view, i) {
nit: if it fits, i think this is purdier

  var ViewType = chrome.developerPrivate.ViewType;
  extension.views.forEach(function(view, i)
    if (view.type == ViewType.EXTENSION_DIALOG || ViewType.EXTENSION_POPUP)
      return;

[Devlin, 2015-03-12 15:38:14]

https://codereview.chromium.org/989813002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/989813002/diff/100001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:731:
extension.views.forEach(function(view, i) {
On 2015/03/12 00:23:15, Dan Beam wrote:
> nit: if it fits, i think this is purdier
> 
>   var ViewType = chrome.developerPrivate.ViewType;
>   extension.views.forEach(function(view, i)
>     if (view.type == ViewType.EXTENSION_DIALOG || ViewType.EXTENSION_POPUP)
>       return;

But you're missing the second view.type == - should be
if (view.type == ViewType.EXTENSION_DIALOG ||
    view.type == ViewType.EXTENSION_POPUP)
which then doesn't save any lines.  Do you still have a preference for caching
it?

[Mark P, 2015-03-12 16:24:35]

histograms.xml lgtm

[Dan Beam, 2015-03-12 16:41:17]

lgtm

https://codereview.chromium.org/989813002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/989813002/diff/100001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:731:
extension.views.forEach(function(view, i) {
On 2015/03/12 15:38:14, Devlin wrote:
> On 2015/03/12 00:23:15, Dan Beam wrote:
> > nit: if it fits, i think this is purdier
> > 
> >   var ViewType = chrome.developerPrivate.ViewType;
> >   extension.views.forEach(function(view, i)
> >     if (view.type == ViewType.EXTENSION_DIALOG || ViewType.EXTENSION_POPUP)
> >       return;
> 
> But you're missing the second view.type == - should be
> if (view.type == ViewType.EXTENSION_DIALOG ||
>     view.type == ViewType.EXTENSION_POPUP)
> which then doesn't save any lines.  Do you still have a preference for caching
> it?

oh, doh.  if it doesn't save lines don't bother, but add curlies

[Devlin, 2015-03-12 18:03:08]

Patchset #6 (id:120001) has been deleted

[Devlin, 2015-03-12 18:19:10]

Patchset #6 (id:140001) has been deleted

[Devlin, 2015-03-12 19:34:15]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-03-12 19:34:16]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org,
mpearson@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/989813002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-03-12 19:34:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/989813002/160001

[Devlin, 2015-03-12 19:53:35]

https://codereview.chromium.org/989813002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/989813002/diff/100001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:731:
extension.views.forEach(function(view, i) {
On 2015/03/12 16:41:17, Dan Beam wrote:
> On 2015/03/12 15:38:14, Devlin wrote:
> > On 2015/03/12 00:23:15, Dan Beam wrote:
> > > nit: if it fits, i think this is purdier
> > > 
> > >   var ViewType = chrome.developerPrivate.ViewType;
> > >   extension.views.forEach(function(view, i)
> > >     if (view.type == ViewType.EXTENSION_DIALOG ||
ViewType.EXTENSION_POPUP)
> > >       return;
> > 
> > But you're missing the second view.type == - should be
> > if (view.type == ViewType.EXTENSION_DIALOG ||
> >     view.type == ViewType.EXTENSION_POPUP)
> > which then doesn't save any lines.  Do you still have a preference for
caching
> > it?
> 
> oh, doh.  if it doesn't save lines don't bother, but add curlies

Done.

[commit-bot: I haz the power, 2015-03-12 20:12:01]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-03-12 20:12:56]

Patchset 6 (id:??) landed as
https://crrev.com/609f7a872624668960c7da22221924c47a8f885f
Cr-Commit-Position: refs/heads/master@{#320355}