Docserver: Display API features that are available to content scripts

Docserver: Display API features that are available to content scripts

BUG=278919
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284523

[ahernandez, 2014-07-09 01:06:26]

Here is where DanielJ left off on displaying API feature availability within
content scripts. It seems like the logic he left is missing something, but he
didn't leave me any documentation on this patch so I'm not sure how exactly the
data is supposed to be manipulated/presented.

[not at google - send to devlin, 2014-07-09 02:55:55]

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/api_data_source.py:537:
'content_scripts.html')).Get(),
this doesn't actually exist

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/api_data_source_test.py:254: model =
_JSCModel(self._api_models,
valign

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/api_models.py (right):

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/api_models.py:103: def
GetContentScriptsAPIs(self):
s/GetContentScriptsAPIs/GetContentScriptAPIs/

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/api_models.py:109: 'name': 'extension',
Can we return an actual type rather than a dictionary?

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/api_models.py:110: 'fullSupport': False,
'allNodes' might be more consistent with 'supportedNodes'

even so having 2 properties here seems a bit icky. maybe if you were to think of
it as "the restricted set of nodes" such that None would be "nothing
restricted".

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/api_models.py:125: api_features =
api_features_future.Get()
you don't need to resolve this is if content_script_apis is not None, so move it
after that.

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/api_models.py:132: if 'content_script' in
feature.get('contexts', ()):
early-continue

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/templates/articles/content_scripts.html
(right):

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/articles/content_scripts.html:30: Use
chrome.* APIs, with the exception of:
this is really interesting. it's uncovered several bugs!

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/articles/content_scripts.html:32:
{{#api:features}}
"content_scripts" would be better than "features".

[not at google - send to devlin, 2014-07-09 03:03:00]

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/api_list_data_source.py:39: return
content_scripts_apis_list
we should filter out undocumented APIs here, much as we do for the normal API
list.

[ahernandez, 2014-07-09 19:22:36]

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/templates/articles/content_scripts.html
(right):

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/articles/content_scripts.html:30: Use
chrome.* APIs, with the exception of:
On 2014/07/09 02:55:55, kalman wrote:
> this is really interesting. it's uncovered several bugs!

Are APIs available to content scripts that shouldn't be?

[ahernandez, 2014-07-09 19:42:21]

PTAL

[not at google - send to devlin, 2014-07-09 20:49:53]

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/templates/articles/content_scripts.html
(right):

https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/templates/articles/content_scripts.html:30: Use
chrome.* APIs, with the exception of:
On 2014/07/09 19:22:36, ahernandez wrote:
> On 2014/07/09 02:55:55, kalman wrote:
> > this is really interesting. it's uncovered several bugs!
> 
> Are APIs available to content scripts that shouldn't be?

yup

[not at google - send to devlin, 2014-07-10 16:38:13]

On 2014/07/09 20:49:53, kalman wrote:
>
https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
> File chrome/common/extensions/docs/templates/articles/content_scripts.html
> (right):
> 
>
https://codereview.chromium.org/375133002/diff/1/chrome/common/extensions/doc...
> chrome/common/extensions/docs/templates/articles/content_scripts.html:30: Use
> chrome.* APIs, with the exception of:
> On 2014/07/09 19:22:36, ahernandez wrote:
> > On 2014/07/09 02:55:55, kalman wrote:
> > > this is really interesting. it's uncovered several bugs!
> > 
> > Are APIs available to content scripts that shouldn't be?
> 
> yup

https://codereview.chromium.org/375173002

[not at google - send to devlin, 2014-07-10 17:31:28]

almost there.

https://codereview.chromium.org/375133002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_models.py (right):

https://codereview.chromium.org/375133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_models.py:25: self.restrictedTo = []
I'd rather "nothing restricted" be modelled as None, not empty-list, if you can
manage it in a clean way.

https://codereview.chromium.org/375133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_models.py:27: def get(self, key):
exposing a "get" method is an implementation detail of the data sources, not of
the model.

and note that you can easily do all of this stuff by accessing the __dict__
property of these things. like,

content_script_apis = [csa.__dict__
                       for csa in api_models.GetContentScriptAPIs()]

and sort, MarkFirst/Last however you want.

https://codereview.chromium.org/375133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_models.py:162: # Sort all of the
supportedNodes.
see comment at top of file.

https://codereview.chromium.org/375133002/diff/20001/chrome/common/extensions...
File
chrome/common/extensions/docs/templates/private/intro_tables/content_scripts.html
(right):

https://codereview.chromium.org/375133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_tables/content_scripts.html:1:
{{^content.contentScriptSupport.restrictedTo}}
could you reverse the order of these checks? I'd prefer to see the
positive-match first (?) rather than the negative.

also... I think you can do:

{{?restrictedTo:content.contentScriptSupport.restrictedTo}}
  {{#n:restrictedTo}}
  ...
  {{/n:restrictedTo}}
{{:}}
  ...
{{/contentScriptSupport.restrictedTo}}

i.e. bind contentScriptSupport.restrictedTo to restrictedTo. Pretty sure I
implemented that:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/handle...

https://codereview.chromium.org/375133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_tables/content_scripts.html:2:
Fully supported.
Arbitrary place for this comment: I noticed that the "Content Scripts" wraps in
the i18n API. Could you add:

table#intro {
  .title {
    white-space: nowrap;
  }
}

inside the .api rule in _api.scss? That should fix it.

https://codereview.chromium.org/375133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/intro_tables/content_scripts.html:10:
{{/content.contentScriptSupport.restrictedTo}}
could you add at the bottom here "<a href="content_scripts">Learn more</a>."

[ahernandez, 2014-07-14 18:11:18]

https://codereview.chromium.org/375133002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_models.py (right):

https://codereview.chromium.org/375133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_models.py:27: def get(self, key):
On 2014/07/10 17:31:27, kalman (OOO July 11-14) wrote:
> exposing a "get" method is an implementation detail of the data sources, not
of
> the model.
> 
> and note that you can easily do all of this stuff by accessing the __dict__
> property of these things. like,
> 
> content_script_apis = [csa.__dict__
>                        for csa in api_models.GetContentScriptAPIs()]
> 
> and sort, MarkFirst/Last however you want.

If this doesn't have a get() method, how can handlebar get the right data? I'm
not sure where else I could expose a get() method.

[not at google - send to devlin, 2014-07-15 16:26:38]

https://codereview.chromium.org/375133002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_models.py (right):

https://codereview.chromium.org/375133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_models.py:27: def get(self, key):
On 2014/07/14 18:11:18, ahernandez wrote:
> On 2014/07/10 17:31:27, kalman (OOO July 11-14) wrote:
> > exposing a "get" method is an implementation detail of the data sources, not
> of
> > the model.
> > 
> > and note that you can easily do all of this stuff by accessing the __dict__
> > property of these things. like,
> > 
> > content_script_apis = [csa.__dict__
> >                        for csa in api_models.GetContentScriptAPIs()]
> > 
> > and sort, MarkFirst/Last however you want.
> 
> If this doesn't have a get() method, how can handlebar get the right data? I'm
> not sure where else I could expose a get() method.

__dict__ returns a dict all of the properties of that method, right? and all
dicts have a get() method?

[ahernandez, 2014-07-15 19:30:32]

PTAL.

[not at google - send to devlin, 2014-07-15 21:34:40]

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source_test.py:235:
'contentScriptSupport': expected_content_script_support.__dict__
I'd rather you test the dict output directly rather than relying on whatever
ContentScriptAPI.__dict__ spits out. i.e. what you had before?

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_list_data_source.py:35:
content_scripts_apis = self._platform_bundle.GetAPIModels(
s/content_scripts_apis/content_script_apis/

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_list_data_source.py:37:
content_scripts_apis_list = []
s/content_scripts_apis_list/content_script_api_list/

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_list_data_source.py:41:
content_scripts_apis_list.append(content_script_api)
use a list comprehension to populate content_script_apis_list.

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_list_data_source_test.py (right):

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_list_data_source_test.py:247:
expected_extension_api.restrictedTo = []
constructing a ContentScriptAPI object then converting it to a __dict__ is
overly complicated for this test. just construct the dicts directly.

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_models.py (right):

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_models.py:27: def __eq__(self, o):
when you override __eq__ you also need to override __ne__, but if can be just
"def __ne__(self, o): return not (self == o)"

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_models.py:119: 'extension':
<ContentScriptAPI object>,
maybe define str/repr for ContentScriptAPI...

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_models.py:145: # Store the __dict__
attribute, because Handlebar expects
the dict creation should be only inside the data sources? APIModels is pure
model and doesn't care about dicts or any of that.

https://codereview.chromium.org/375133002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_models.py:166:
MarkLast(restricted_nodes)
this sorting should go into the API list data source, since only it should care
about sorting.

[ahernandez, 2014-07-16 00:11:04]

PTAL.

[not at google - send to devlin, 2014-07-16 00:32:20]

https://codereview.chromium.org/375133002/diff/80001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/375133002/diff/80001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:276:
self._content_script_apis = api_models.GetContentScriptAPIs().Get()
come to think of it, if you pass the content script APIs into here directly then
you can make use of the async fetching that |namespace| would use.

https://codereview.chromium.org/375133002/diff/80001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:753: jsc_model_future =
self._model_cache.Get(object_store_key)
i.e. this async fetching.

https://codereview.chromium.org/375133002/diff/80001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):

https://codereview.chromium.org/375133002/diff/80001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_list_data_source.py:47:
MarkLast(restricted_nodes)
could you

else:
  del csa['restrictedTo']

here. nice to save some memory, though, we almost hardly ever do. might as well
start.

https://codereview.chromium.org/375133002/diff/80001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_models.py (right):

https://codereview.chromium.org/375133002/diff/80001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_models.py:24: self.restrictedTo = None
document these

https://codereview.chromium.org/375133002/diff/80001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_models.py:157: node = {'parent':
parent, 'node': feature['name'][len(parent) + 1:]}
this 'parent' property is still a concern of the templates, right? can you just
make restrictedTo be a list of the features it's restricted to, parent a
property of ContentScriptAPI (since it should be constant for all restrictedTo?)
and let the data source(s) figure it out when they do their transformation from
model -> dict.

[ahernandez, 2014-07-16 20:35:34]

Here are the new changes, PTAL.

https://codereview.chromium.org/375133002/diff/100001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/375133002/diff/100001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_data_source.py:584: if
content_script_support.restrictedTo:
Should this code be moved into a function somewhere, and if so where? It's
pretty much exactly the same as the sorting logic in api_list_data_source.py.

[ahernandez, 2014-07-21 17:20:18]

On 2014/07/16 20:35:34, ahernandez wrote:
> Here are the new changes, PTAL.
> 
>
https://codereview.chromium.org/375133002/diff/100001/chrome/common/extension...
> File chrome/common/extensions/docs/server2/api_data_source.py (right):
> 
>
https://codereview.chromium.org/375133002/diff/100001/chrome/common/extension...
> chrome/common/extensions/docs/server2/api_data_source.py:584: if
> content_script_support.restrictedTo:
> Should this code be moved into a function somewhere, and if so where? It's
> pretty much exactly the same as the sorting logic in api_list_data_source.py.

Ping.

[not at google - send to devlin, 2014-07-21 17:51:20]

lgtm with nits

https://codereview.chromium.org/375133002/diff/100001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/375133002/diff/100001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_data_source.py:584: if
content_script_support.restrictedTo:
On 2014/07/16 20:35:34, ahernandez wrote:
> Should this code be moved into a function somewhere, and if so where? It's
> pretty much exactly the same as the sorting logic in api_list_data_source.py.

maybe, though I can't think of an appropriate place. it seems arbitrary to try
and share it. maybe keep it in mind in case it turns out there needs to be more
code shared?

perhaps as a small concession you could add a MarkFirstAndLast method to at
least reduce that little redundancy.

https://codereview.chromium.org/375133002/diff/100001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):

https://codereview.chromium.org/375133002/diff/100001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_list_data_source.py:37:
content_script_apis_list = [csa.__dict__ for api_name, csa in
nit: everywhere else I see the "in" and "if" of list comprehensions be on the
start of the line not the end.

https://codereview.chromium.org/375133002/diff/100001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_list_data_source.py:101:
api_dict['content_scripts'] = make_list_for_content_scripts()
actually can this be 'contentScripts'? Google JSON style is supposed to be
camelCase, it's just the docserver which chooses arbitrarily. let's try to stick
to camel case from now on.

https://codereview.chromium.org/375133002/diff/100001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_models.py (right):

https://codereview.chromium.org/375133002/diff/100001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_models.py:146: for feature in
api_features.itervalues():
maybe you should do "name, feature in api_features.iteritems()" then use |name|
everywhere instead of |feature['name']|.

https://codereview.chromium.org/375133002/diff/100001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_models.py:149: parent =
GetParentFeature(feature['name'], feature, api_features)
can you rename GetParentFeature to GetParentFeatureName? confused me here for a
bit.

or even just GetParentName since the "feature" is implicit given it's in the
features_bundle file. if so, HasParentFeature should be renamed HasParent.
either way up to you.

[ahernandez, 2014-07-21 18:46:51]

The CQ bit was checked by ahernandez.miralles@gmail.com

[commit-bot: I haz the power, 2014-07-21 18:48:12]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/37513300...

[commit-bot: I haz the power, 2014-07-21 22:39:47]

Change committed as 284523