Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521363002:
[Extensions] Remove ChromeExtensionFunctionDetails::GetOriginWebContents (Closed)
Description[Extensions] Remove ChromeExtensionFunctionDetails::GetOriginWebContents
Remove the ChromeExtensionFunctionDetails::GetOriginWebContents()
method. There are a few problems with this function:
- Like GetAssociatedWebContents(), it is unclear and unpredictable (see
also crbug.com/461394). It's not obvious what the returned result will
be, and it's very likely *not* what the caller is expecting from the
title or function comment.
- It uses ExtensionFunction::source_tab_id, which needs to be removed.
Remove the function and implement the necessary pieces directly in the
previous callers.
BUG=667584
Committed: https://crrev.com/ae224f1bfe3875919fa6c79130a31affb6e01f2f
Cr-Commit-Position: refs/heads/master@{#434097}
Patch Set 1 #
Total comments: 4
Patch Set 2 : lazyboy's #
Messages
Total messages: 24 (13 generated)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== foo BUG= ========== to ========== [Extensions] Remove ChromeExtensionFunctionDetails::GetOriginWebContents Remove the ChromeExtensionFunctionDetails::GetOriginWebContents() method. There are a few problems with this function: - Like GetAssociatedWebContents(), it is unclear and unpredictable (see also crbug.com/461394). It's not obvious what the returned result will be, and it's very likely *not* what the caller is expecting from the title or function comment. - It uses ExtensionFunction::source_tab_id, which needs to be removed. Remove the function and implement the necessary pieces directly in the previous callers. BUG=667584 ==========
rdevlin.cronin@chromium.org changed reviewers: + lazyboy@chromium.org, tommycli@chromium.org
lazyboy@ (extensions) and tommycli@ (media galleries), mind taking a look?
https://codereview.chromium.org/2521363002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/2521363002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:475: GetWebContentsForPrompt(GetSenderWebContents(), Previously the code would check if GetSenderWebContents() is null: chrome/browser/extensions/chrome_extension_function_details.cc:109, should we add this back as WebContentsModalDialogManager::FromWebContents in GetWebContentsForPrompt will try to deref the WebContents*?
lgtm -- I don't know much about the GetOriginWebContents refactor, so I can't comment on that. But assuming that the new code retrieves a WebContents that's more appropriate than the old GetOriginWebContents method, that's a good thing. I see nothing here that will break anything media galleries specific.
https://codereview.chromium.org/2521363002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/2521363002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:271: content::WebContents* GetWebContentsForPrompt( AFAICT, this function basically does: If the sender web contents supports dialogs, use that. Otherwise use the "current" window for the app, which is guaranteed to support modal dialogs. That sounds good - but I am a bit surprised that this function is in the anonymous section of media_galleries_api rather than UIThreadExtensionFunction. It doesn't seem media galleries specific at all... just happens that Media Galleries is the first (or only?) API that needs to display a modal dialog?
https://codereview.chromium.org/2521363002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/2521363002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:271: content::WebContents* GetWebContentsForPrompt( On 2016/11/22 21:59:41, tommycli wrote: > AFAICT, this function basically does: If the sender web contents supports > dialogs, use that. Otherwise use the "current" window for the app, which is > guaranteed to support modal dialogs. > > That sounds good - but I am a bit surprised that this function is in the > anonymous section of media_galleries_api rather than UIThreadExtensionFunction. > > It doesn't seem media galleries specific at all... just happens that Media > Galleries is the first (or only?) API that needs to display a modal dialog? More or less. I'm actually moving it from ChromeExtensionFunctionDetails. Right now, media galleries is the only thing that needs this implementation, and the implementation changes depending on e.g. whether or not the calling item is an extension or an app, whether or not using a foreground tab or the browser window is acceptable, etc. Additionally, a name like GetOriginWebContents() really doesn't describe what it's doing, and doesn't cover these questions of acceptable criteria. I could see in the future using a variant of GetNativeWindowForUI(), but I don't think that's quite right here either, for the same reasons of ambiguity around what's acceptable. If/when we clean that up, it's probably what we'd want. https://codereview.chromium.org/2521363002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:475: GetWebContentsForPrompt(GetSenderWebContents(), On 2016/11/22 20:47:41, lazyboy wrote: > Previously the code would check if GetSenderWebContents() is null: > > chrome/browser/extensions/chrome_extension_function_details.cc:109, should we > add this back as WebContentsModalDialogManager::FromWebContents in > GetWebContentsForPrompt will try to deref the WebContents*? Good call, now checking in GetWebContentsForPrompt.
lgtm
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2521363002/#ps20001 (title: "lazyboy's")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479867720465220,
"parent_rev": "897bd925dd96a19b758d76904ecc276789d976c3", "commit_rev":
"673a9eea196bc1e0e4a7b248d0c589578ace119b"}
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Remove ChromeExtensionFunctionDetails::GetOriginWebContents Remove the ChromeExtensionFunctionDetails::GetOriginWebContents() method. There are a few problems with this function: - Like GetAssociatedWebContents(), it is unclear and unpredictable (see also crbug.com/461394). It's not obvious what the returned result will be, and it's very likely *not* what the caller is expecting from the title or function comment. - It uses ExtensionFunction::source_tab_id, which needs to be removed. Remove the function and implement the necessary pieces directly in the previous callers. BUG=667584 ========== to ========== [Extensions] Remove ChromeExtensionFunctionDetails::GetOriginWebContents Remove the ChromeExtensionFunctionDetails::GetOriginWebContents() method. There are a few problems with this function: - Like GetAssociatedWebContents(), it is unclear and unpredictable (see also crbug.com/461394). It's not obvious what the returned result will be, and it's very likely *not* what the caller is expecting from the title or function comment. - It uses ExtensionFunction::source_tab_id, which needs to be removed. Remove the function and implement the necessary pieces directly in the previous callers. BUG=667584 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Remove ChromeExtensionFunctionDetails::GetOriginWebContents Remove the ChromeExtensionFunctionDetails::GetOriginWebContents() method. There are a few problems with this function: - Like GetAssociatedWebContents(), it is unclear and unpredictable (see also crbug.com/461394). It's not obvious what the returned result will be, and it's very likely *not* what the caller is expecting from the title or function comment. - It uses ExtensionFunction::source_tab_id, which needs to be removed. Remove the function and implement the necessary pieces directly in the previous callers. BUG=667584 ========== to ========== [Extensions] Remove ChromeExtensionFunctionDetails::GetOriginWebContents Remove the ChromeExtensionFunctionDetails::GetOriginWebContents() method. There are a few problems with this function: - Like GetAssociatedWebContents(), it is unclear and unpredictable (see also crbug.com/461394). It's not obvious what the returned result will be, and it's very likely *not* what the caller is expecting from the title or function comment. - It uses ExtensionFunction::source_tab_id, which needs to be removed. Remove the function and implement the necessary pieces directly in the previous callers. BUG=667584 Committed: https://crrev.com/ae224f1bfe3875919fa6c79130a31affb6e01f2f Cr-Commit-Position: refs/heads/master@{#434097} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ae224f1bfe3875919fa6c79130a31affb6e01f2f Cr-Commit-Position: refs/heads/master@{#434097} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
