Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12377047:
Add chrome.debugger.getTargets method to discover available debug targets. (Closed)
|
Created:
7 years, 9 months ago by Vladislav Kaznacheev Modified:
7 years, 8 months ago CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@targets Visibility:
Public. |
DescriptionAdd chrome.debugger.getTargets method to discover available debug targets.
BUG=179342
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191151
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191982
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebased #
Total comments: 10
Patch Set 4 : Rebased, addressed comments #
Total comments: 10
Patch Set 5 : Addressed comments #Patch Set 6 : Refactored infobar checks #Patch Set 7 : Rebase #
Total comments: 4
Patch Set 8 : Fixed the tests #Patch Set 9 : Removed flaky tests #
Messages
Total messages: 23 (0 generated)
Please review
This is now ready for review.
https://codereview.chromium.org/12377047/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/12377047/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/debugger/debugger_api.cc:208: base::DictionaryValue* dictionary = new base::DictionaryValue; new base::DictionaryValue() https://codereview.chromium.org/12377047/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/debugger/debugger_api.cc:210: dictionary->SetString("id", agent_host->GetId()); You should use constants for these. https://codereview.chromium.org/12377047/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/debugger/debugger_api.cc:216: dictionary->SetString("type", "extension"); I don't think we are ready to finalize this field. https://codereview.chromium.org/12377047/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/debugger/debugger_api.cc:489: debuggee_.extension_id ? Consider extracting this. https://codereview.chromium.org/12377047/diff/11001/chrome/common/extensions/... File chrome/common/extensions/api/debugger.json (right): https://codereview.chromium.org/12377047/diff/11001/chrome/common/extensions/... chrome/common/extensions/api/debugger.json:25: "type": { "type": "string", "description": "Target type ('page' or 'extension')." }, If we are declaring it public, we need to declare it as enum with all values thought through.
PTAL https://codereview.chromium.org/12377047/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/12377047/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/debugger/debugger_api.cc:208: base::DictionaryValue* dictionary = new base::DictionaryValue; On 2013/03/15 11:25:45, pfeldman wrote: > new base::DictionaryValue() Done. https://codereview.chromium.org/12377047/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/debugger/debugger_api.cc:210: dictionary->SetString("id", agent_host->GetId()); On 2013/03/15 11:25:45, pfeldman wrote: > You should use constants for these. Done. https://codereview.chromium.org/12377047/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/debugger/debugger_api.cc:216: dictionary->SetString("type", "extension"); Do you mean you prefer this called "other"? Please elaborate. On 2013/03/15 11:25:45, pfeldman wrote: > I don't think we are ready to finalize this field. https://codereview.chromium.org/12377047/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/debugger/debugger_api.cc:489: debuggee_.extension_id ? On 2013/03/15 11:25:45, pfeldman wrote: > Consider extracting this. Done. https://codereview.chromium.org/12377047/diff/11001/chrome/common/extensions/... File chrome/common/extensions/api/debugger.json (right): https://codereview.chromium.org/12377047/diff/11001/chrome/common/extensions/... chrome/common/extensions/api/debugger.json:25: "type": { "type": "string", "description": "Target type ('page' or 'extension')." }, On 2013/03/15 11:25:45, pfeldman wrote: > If we are declaring it public, we need to declare it as enum with all values > thought through. Done.
devtools lgtm
Hello Matt, Please review the changes to: chrome/browser/extensions/api/debugger/debugger_api_constants.h chrome/browser/extensions/api/debugger/debugger_api_constants.cc chrome/browser/extensions/api/debugger/debugger_api.h chrome/browser/extensions/api/debugger/debugger_api.cc Thanks, Vlad
I forgot to mention chrome/common/extensions/api/debugger.json which needs reviewing as well. > Hello Matt, > > Please review the changes to: > chrome/browser/extensions/api/debugger/debugger_api_constants.h > chrome/browser/extensions/api/debugger/debugger_api_constants.cc > chrome/browser/extensions/api/debugger/debugger_api.h > chrome/browser/extensions/api/debugger/debugger_api.cc > > Thanks, > > Vlad
https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api.cc:224: GetExtensionBackgroundHost(web_contents); This is a very roundabout way to get the extension's name. It also won't work for popups or other views. If you just want the name, use ExtensionService::extensions()->GetExtensionOrAppByURL(url). https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api.cc:238: if (web_contents) { Can web_contents actually be NULL? You use it in GetExtensionBackgroundHost without NULL-checking it. https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api.cc:563: // Allow only tabs, reject background pages. Why? And what about popups or extension pages within a tab?
Thanks for the comments, PTAL. https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api.cc:224: GetExtensionBackgroundHost(web_contents); It is not only only about the name. I need to know if the RenderViewHost belongs to an extension background page. If so I use the extension name as a title (and also set "extension" as a type). Otherwise I use the page title as a title (and set "page" as a type). Added comments to both if branches. Do you think that "background" would be a better name for a non-page debug target than "extension"? On 2013/03/18 19:53:44, Matt Perry wrote: > This is a very roundabout way to get the extension's name. It also won't work > for popups or other views. If you just want the name, use > ExtensionService::extensions()->GetExtensionOrAppByURL(url). https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api.cc:238: if (web_contents) { You are right. This came from inspect_ui.cc where similar code exists. The comment for WebContents::FromRenderViewHost says the return value can be NULL. This would make the entire function useless, so I moved the check to the top of the function. On 2013/03/18 19:53:44, Matt Perry wrote: > Can web_contents actually be NULL? You use it in GetExtensionBackgroundHost > without NULL-checking it. https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api.cc:563: // Allow only tabs, reject background pages. When an extension attaches to the page using chrome.debugger API we must show a warning infobar (for security reasons). When attaching to a background page there is nowhere we can show an infobar, so when this feature was first supportd (https://chromiumcodereview.appspot.com/12304021) in a decision has been made to allow this only if the infobar is explicitly suppressed using --silent-debugger-extension-api flag. I agree that the comment is too cryptic, rewrote it for clarity. On 2013/03/18 19:53:44, Matt Perry wrote: > Why? And what about popups or extension pages within a tab?
https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api.cc:224: GetExtensionBackgroundHost(web_contents); OK, so you really do only want this to apply to background pages. In that case, "background_page" would be a better name. On 2013/03/19 05:56:23, Vladislav Kaznacheev wrote: > It is not only only about the name. I need to know if the RenderViewHost belongs > to an extension background page. If so I use the extension name as a title (and > also set "extension" as a type). Otherwise I use the page title as a title (and > set "page" as a type). Added comments to both if branches. > > Do you think that "background" would be a better name for a non-page debug > target than "extension"? > > > > On 2013/03/18 19:53:44, Matt Perry wrote: > > This is a very roundabout way to get the extension's name. It also won't work > > for popups or other views. If you just want the name, use > > ExtensionService::extensions()->GetExtensionOrAppByURL(url). > https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api.cc:563: // Allow only tabs, reject background pages. Might this apply to extension popups as well? If not, where would you show the infobar in that case? On 2013/03/19 05:56:23, Vladislav Kaznacheev wrote: > When an extension attaches to the page using chrome.debugger API we must show a > warning infobar (for security reasons). > > When attaching to a background page there is nowhere we can show an infobar, so > when this feature was first supportd > (https://chromiumcodereview.appspot.com/12304021) in a decision has been made to > allow this only if the infobar is explicitly suppressed using > --silent-debugger-extension-api flag. > > I agree that the comment is too cryptic, rewrote it for clarity. > > On 2013/03/18 19:53:44, Matt Perry wrote: > > Why? And what about popups or extension pages within a tab? >
PTAL https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api.cc:224: GetExtensionBackgroundHost(web_contents); On 2013/03/19 18:02:01, Matt Perry wrote: > OK, so you really do only want this to apply to background pages. In that case, > "background_page" would be a better name. > > On 2013/03/19 05:56:23, Vladislav Kaznacheev wrote: > > It is not only only about the name. I need to know if the RenderViewHost > belongs > > to an extension background page. If so I use the extension name as a title > (and > > also set "extension" as a type). Otherwise I use the page title as a title > (and > > set "page" as a type). Added comments to both if branches. > > > > Do you think that "background" would be a better name for a non-page debug > > target than "extension"? > > > > > > > > On 2013/03/18 19:53:44, Matt Perry wrote: > > > This is a very roundabout way to get the extension's name. It also won't > work > > > for popups or other views. If you just want the name, use > > > ExtensionService::extensions()->GetExtensionOrAppByURL(url). > > > Done. https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api.cc:563: // Allow only tabs, reject background pages. Yes, I missed that. I have refactored the infobar handling. Instead of guessing if it is OK to create the infobar I actually attempt to create the infobar before attaching to the target. If the infobar cannot be created for any reason I return an error. On 2013/03/19 18:02:01, Matt Perry wrote: > Might this apply to extension popups as well? If not, where would you show the > infobar in that case? > > On 2013/03/19 05:56:23, Vladislav Kaznacheev wrote: > > When an extension attaches to the page using chrome.debugger API we must show > a > > warning infobar (for security reasons). > > > > When attaching to a background page there is nowhere we can show an infobar, > so > > when this feature was first supportd > > (https://chromiumcodereview.appspot.com/12304021) in a decision has been made > to > > allow this only if the infobar is explicitly suppressed using > > --silent-debugger-extension-api flag. > > > > I agree that the comment is too cryptic, rewrote it for clarity. > > > > On 2013/03/18 19:53:44, Matt Perry wrote: > > > Why? And what about popups or extension pages within a tab? > > >
Gentle ping On 2013/03/20 07:20:49, Vladislav Kaznacheev wrote: > PTAL > > https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... > File chrome/browser/extensions/api/debugger/debugger_api.cc (right): > > https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... > chrome/browser/extensions/api/debugger/debugger_api.cc:224: > GetExtensionBackgroundHost(web_contents); > On 2013/03/19 18:02:01, Matt Perry wrote: > > OK, so you really do only want this to apply to background pages. In that > case, > > "background_page" would be a better name. > > > > On 2013/03/19 05:56:23, Vladislav Kaznacheev wrote: > > > It is not only only about the name. I need to know if the RenderViewHost > > belongs > > > to an extension background page. If so I use the extension name as a title > > (and > > > also set "extension" as a type). Otherwise I use the page title as a title > > (and > > > set "page" as a type). Added comments to both if branches. > > > > > > Do you think that "background" would be a better name for a non-page debug > > > target than "extension"? > > > > > > > > > > > > On 2013/03/18 19:53:44, Matt Perry wrote: > > > > This is a very roundabout way to get the extension's name. It also won't > > work > > > > for popups or other views. If you just want the name, use > > > > ExtensionService::extensions()->GetExtensionOrAppByURL(url). > > > > > > > Done. > > https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... > chrome/browser/extensions/api/debugger/debugger_api.cc:563: // Allow only tabs, > reject background pages. > Yes, I missed that. I have refactored the infobar handling. Instead of guessing > if it is OK to create the infobar I actually attempt to create the infobar > before attaching to the target. If the infobar cannot be created for any reason > I return an error. > > On 2013/03/19 18:02:01, Matt Perry wrote: > > Might this apply to extension popups as well? If not, where would you show the > > infobar in that case? > > > > On 2013/03/19 05:56:23, Vladislav Kaznacheev wrote: > > > When an extension attaches to the page using chrome.debugger API we must > show > > a > > > warning infobar (for security reasons). > > > > > > When attaching to a background page there is nowhere we can show an infobar, > > so > > > when this feature was first supportd > > > (https://chromiumcodereview.appspot.com/12304021) in a decision has been > made > > to > > > allow this only if the infobar is explicitly suppressed using > > > --silent-debugger-extension-api flag. > > > > > > I agree that the comment is too cryptic, rewrote it for clarity. > > > > > > On 2013/03/18 19:53:44, Matt Perry wrote: > > > > Why? And what about popups or extension pages within a tab? > > > > >
Hi Matt, can you take another look please? Vlad On 2013/03/22 05:45:27, Vladislav Kaznacheev wrote: > Gentle ping > > On 2013/03/20 07:20:49, Vladislav Kaznacheev wrote: > > PTAL > > > > > https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... > > File chrome/browser/extensions/api/debugger/debugger_api.cc (right): > > > > > https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... > > chrome/browser/extensions/api/debugger/debugger_api.cc:224: > > GetExtensionBackgroundHost(web_contents); > > On 2013/03/19 18:02:01, Matt Perry wrote: > > > OK, so you really do only want this to apply to background pages. In that > > case, > > > "background_page" would be a better name. > > > > > > On 2013/03/19 05:56:23, Vladislav Kaznacheev wrote: > > > > It is not only only about the name. I need to know if the RenderViewHost > > > belongs > > > > to an extension background page. If so I use the extension name as a title > > > (and > > > > also set "extension" as a type). Otherwise I use the page title as a title > > > (and > > > > set "page" as a type). Added comments to both if branches. > > > > > > > > Do you think that "background" would be a better name for a non-page debug > > > > target than "extension"? > > > > > > > > > > > > > > > > On 2013/03/18 19:53:44, Matt Perry wrote: > > > > > This is a very roundabout way to get the extension's name. It also won't > > > work > > > > > for popups or other views. If you just want the name, use > > > > > ExtensionService::extensions()->GetExtensionOrAppByURL(url). > > > > > > > > > > > Done. > > > > > https://chromiumcodereview.appspot.com/12377047/diff/23001/chrome/browser/ext... > > chrome/browser/extensions/api/debugger/debugger_api.cc:563: // Allow only > tabs, > > reject background pages. > > Yes, I missed that. I have refactored the infobar handling. Instead of > guessing > > if it is OK to create the infobar I actually attempt to create the infobar > > before attaching to the target. If the infobar cannot be created for any > reason > > I return an error. > > > > On 2013/03/19 18:02:01, Matt Perry wrote: > > > Might this apply to extension popups as well? If not, where would you show > the > > > infobar in that case? > > > > > > On 2013/03/19 05:56:23, Vladislav Kaznacheev wrote: > > > > When an extension attaches to the page using chrome.debugger API we must > > show > > > a > > > > warning infobar (for security reasons). > > > > > > > > When attaching to a background page there is nowhere we can show an > infobar, > > > so > > > > when this feature was first supportd > > > > (https://chromiumcodereview.appspot.com/12304021) in a decision has been > > made > > > to > > > > allow this only if the infobar is explicitly suppressed using > > > > --silent-debugger-extension-api flag. > > > > > > > > I agree that the comment is too cryptic, rewrote it for clarity. > > > > > > > > On 2013/03/18 19:53:44, Matt Perry wrote: > > > > > Why? And what about popups or extension pages within a tab? > > > > > > >
Sorry for the delay, I somehow missed your pings. LGTM https://chromiumcodereview.appspot.com/12377047/diff/40001/chrome/browser/ext... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://chromiumcodereview.appspot.com/12377047/diff/40001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api.cc:215: static const char kTargetTypeBackgroundPage[] = "background_page"; this is redundant with the debugger_api_constants, isn't it? https://chromiumcodereview.appspot.com/12377047/diff/40001/chrome/browser/ext... File chrome/browser/extensions/api/debugger/debugger_api_constants.cc (right): https://chromiumcodereview.appspot.com/12377047/diff/40001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api_constants.cc:27: const char kBackgroundPageTargetType[] = "background page"; background_page
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/12377047/58001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/12377047/58001
Message was sent while issue was closed.
Change committed as 191151
Message was sent while issue was closed.
Thanks for the review! https://chromiumcodereview.appspot.com/12377047/diff/40001/chrome/browser/ext... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://chromiumcodereview.appspot.com/12377047/diff/40001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api.cc:215: static const char kTargetTypeBackgroundPage[] = "background_page"; Not really. This one is sort of an enum value, the other one is a snippet for an error message. On 2013/03/27 23:29:56, Matt Perry wrote: > this is redundant with the debugger_api_constants, isn't it? https://chromiumcodereview.appspot.com/12377047/diff/40001/chrome/browser/ext... File chrome/browser/extensions/api/debugger/debugger_api_constants.cc (right): https://chromiumcodereview.appspot.com/12377047/diff/40001/chrome/browser/ext... chrome/browser/extensions/api/debugger/debugger_api_constants.cc:27: const char kBackgroundPageTargetType[] = "background page"; I can see how this can be confusing. This string is substituted into messages ("No background page with given id") so the underscore does not belong here. On 2013/03/27 23:29:56, Matt Perry wrote: > background_page
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/12377047/83002
Retried try job too often on win_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/12377047/83002
Message was sent while issue was closed.
Change committed as 191982 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
