Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
283603003:
fixes for issue #51 in devtools-docs (Closed)
|
Created:
6 years, 7 months ago by mem Modified:
6 years, 5 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description
[[ issue closed. picked back up over at https://chromiumcodereview.appspot.com/391243004/ ]]
BUG=
Patch Set 1 #Patch Set 2 : fixed some problems after figuring out how to preview the files #
Total comments: 18
Patch Set 3 : changes based on comments by caseq #
Total comments: 2
Messages
Total messages: 14 (0 generated)
Hello: I fixed a few bugs in the devtools docs (panels.json and inspected_window.json). It's staged at the link below, but my changes aren't showing up. Any thoughts? Thanks. mem https://chrome-apps-doc.appspot.com/_patch/283603003/extensions/devtools_panels
_patch isn't working at the moment, does it look right on preview.py though?
On 2014/05/12 21:42:28, kalman wrote: > _patch isn't working at the moment, does it look right on preview.py though? I have a sparse checkout so I don't have all of the modules required by preview.py. When is _patch back up? Thanks mem
I don't have time to work on it, so unclear.
can you check out chrome/common/extensions? that will include everything you need.
I figured out how to run preview so I was able to fix some issues that I had with the JSON formatting for tables. Enjoy. mem
https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... File chrome/common/extensions/api/devtools/inspected_window.json (right): https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:102: "description": "The options parameter can contain one or more of three options.", nit: can we drop the exact options count? I bet we'll forget to update it when we add more :) https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:107: "description": "Specify a frame other than the inspected window's top frame." If specified, the expression is evaluated on the iframe whose url matches the one specified. By default, the expression is evaluated in the top frame of the inspected page. https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:112: "description": "Evaluate the expression in the JS context with the same Web Origin as this extension. If this extension has no content script, the result will be a console.warn message." "Evaluate the expression in the context of the content script of calling extension, provided such content script is already injected into the inspected page. If not, the expression will not be evaluated and the callback will be invoked with the exception parameter set to object that has <code>isError</code> field set to true and <code>code</code> field set to <code>E_NOTFOUND</code>. I think whenever we talk of the errors, we should make emphasis the API aspect (i.e. describe under what conditions and in what ways we indicate errors to the extension). The console.warn is somewhat less important to document, since it's somewhat self-evident. The error handing in the particular case of eval is even more confusing, since we may report either DevTools-side error (such as the one described above), or a JavaScript exception that occurred while actually evaluating the expression. I think we may want to describe the difference between two somewhere in the eval() usage comments or in the description of callback (in both cases, the value parameter of the callback will be <code>undefined</code> and there will be a non-null exception parameter that will have <code>isError</code> set to true and <code>code</code> set to error code in the former case and <code>isException</code> set to true and value set to the string value of thrown exception in the latter case). https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:117: "description": "Evaluate the expression in the JS environment with a WebOrigin matching the given string. If given, the contextSecurityOrigin overrides the 'true' setting on userContentScriptContext." ... in the context of a content script of an extension that matches the specified origin. The latter statement is not true according to https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:134: "name": "isException", Let's also rename this to exception or exceptionInfo, since it's no longer a bool. https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:136: "additionalProperties": {"type": "any"}, We may want to document some properties. isErorr, code, description and details will be set if the error occurred on the DevTools side before the expression is evaluated; isException and value will be set if the evaluated code produces an unhandled exception. https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:171: "description": "If specified, this is a JavaScript function that takes three string arguments: the source to preprocess, the URL of the source, and a function name if the source is an DOM event handler. The preprocessorerScript function should return a string to be compiled by Chrome in place of the input source. In the case that the source is a DOM event handler, the returned source must compile to a single JS function." "If specified, this is a script that evaluates into a function that accepts three string arguments ..." https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... File chrome/common/extensions/api/devtools/panels.json (right): https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/panels.json:324: "description": "Allows extensions to open a URL in a Developer Tools panel.", "Requests DevTools to open a URL..." https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/panels.json:345: "name": "applyStyleSheet", Let's not make this one public yet, I'm not sure it's the right way for what it's trying to do.
Thanks for your comments. Changes below. mem https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... File chrome/common/extensions/api/devtools/inspected_window.json (right): https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:102: "description": "The options parameter can contain one or more of three options.", On 2014/05/15 14:31:34, caseq wrote: > nit: can we drop the exact options count? I bet we'll forget to update it when > we add more :) Done. https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:107: "description": "Specify a frame other than the inspected window's top frame." On 2014/05/15 14:31:34, caseq wrote: > If specified, the expression is evaluated on the iframe whose url matches the > one specified. By default, the expression is evaluated in the top frame of the > inspected page. Done. https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:112: "description": "Evaluate the expression in the JS context with the same Web Origin as this extension. If this extension has no content script, the result will be a console.warn message." On 2014/05/15 14:31:34, caseq wrote: > "Evaluate the expression in the context of the content script of calling > extension, provided such content script is already injected into the inspected > page. If not, the expression will not be evaluated and the callback will be > invoked with the exception parameter set to object that has <code>isError</code> > field set to true and <code>code</code> field set to <code>E_NOTFOUND</code>. > > I think whenever we talk of the errors, we should make emphasis the API aspect > (i.e. describe under what conditions and in what ways we indicate errors to the > extension). The console.warn is somewhat less important to document, since it's > somewhat self-evident. > > The error handing in the particular case of eval is even more confusing, since > we may report either DevTools-side error (such as the one described above), or a > JavaScript exception that occurred while actually evaluating the expression. I > think we may want to describe the difference between two somewhere in the eval() > usage comments or in the description of callback (in both cases, the value > parameter of the callback will be <code>undefined</code> and there will be a > non-null exception parameter that will have <code>isError</code> set to true and > <code>code</code> set to error code in the former case and > <code>isException</code> set to true and value set to the string value of thrown > exception in the latter case). Done. https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:117: "description": "Evaluate the expression in the JS environment with a WebOrigin matching the given string. If given, the contextSecurityOrigin overrides the 'true' setting on userContentScriptContext." On 2014/05/15 14:31:34, caseq wrote: > ... in the context of a content script of an extension that matches the > specified origin. > > The latter statement is not true according to > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Done. https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:134: "name": "isException", On 2014/05/15 14:31:34, caseq wrote: > Let's also rename this to exception or exceptionInfo, since it's no longer a > bool. Done. https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:136: "additionalProperties": {"type": "any"}, On 2014/05/15 14:31:34, caseq wrote: > We may want to document some properties. > isErorr, code, description and details will be set if the error occurred on the > DevTools side before the expression is evaluated; > isException and value will be set if the evaluated code produces an unhandled > exception. Done. https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:171: "description": "If specified, this is a JavaScript function that takes three string arguments: the source to preprocess, the URL of the source, and a function name if the source is an DOM event handler. The preprocessorerScript function should return a string to be compiled by Chrome in place of the input source. In the case that the source is a DOM event handler, the returned source must compile to a single JS function." On 2014/05/15 14:31:34, caseq wrote: > "If specified, this is a script that evaluates into a function that accepts > three string arguments ..." Done. https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... File chrome/common/extensions/api/devtools/panels.json (right): https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/panels.json:324: "description": "Allows extensions to open a URL in a Developer Tools panel.", On 2014/05/15 14:31:34, caseq wrote: > "Requests DevTools to open a URL..." Done. https://chromiumcodereview.appspot.com/283603003/diff/20001/chrome/common/ext... chrome/common/extensions/api/devtools/panels.json:345: "name": "applyStyleSheet", On 2014/05/15 14:31:34, caseq wrote: > Let's not make this one public yet, I'm not sure it's the right way for what > it's trying to do. Done.
caseq, can we get one more review? gracias
On 2014/06/02 17:18:14, paulirish wrote: > caseq, can we get one more review? gracias btw _patch is fixed now, though be prepared for extreme slowness.
lgtm, sorry about the delay! https://chromiumcodereview.appspot.com/283603003/diff/40001/chrome/common/ext... File chrome/common/extensions/api/devtools/inspected_window.json (right): https://chromiumcodereview.appspot.com/283603003/diff/40001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:91: "description": "Evaluates a JavaScript expression in the context of the main frame of the inspected page. The expression must evaluate to a JSON-compliant object, otherwise an exception is thrown. The eval function can report either a DevTools-side error or a JavaScript exception that occurs during evaluation. In either case, the <code>result</code> parameter of the callback is <code>undefined</code>. In the case of a DevTools-side error, the <code>isException</code> parameter is non-null and has <code>isError</code> set to true and <code>code</code> set to an error code. In the case of a JavaScript error, <code>isException</code> is set to true and <code>result</code> is set to the string value of thrown.", "is set to the string value of thrown object.", perhaps? https://chromiumcodereview.appspot.com/283603003/diff/40001/chrome/common/ext... chrome/common/extensions/api/devtools/inspected_window.json:151: "type": "string", "type": "arryay", "items": { type: "any" }, "description": "Set if the error occurred on the DevTools side before the expression is evaluated, contains the array of the values that may be substituted into the description string to provide more information about the cause of the error".
mem, do you have some time to address the two nits and land the CL? Excited.
ping mem
Message was sent while issue was closed.
Due to a funny git client we've resubmitted this CL over at https://chromiumcodereview.appspot.com/391243004/ Thanks all! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
