Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2492863003:
[Extensions] Don't show the pending URL for chrome.tabs API navigations (Closed)
Description[Extensions] Don't show the pending URL for chrome.tabs API navigations
For the pdf extension, treat navigations as renderer-initiated. Ideally,
we want this to become default for all extensions, but that risks
breakage.
Based on creis@'s patch at https://codereview.chromium.org/2475033002/.
BUG=660498
TEST=See bug for repro steps.
Committed: https://crrev.com/2097de33a1f3d04e78d93e9e2f16aad4b97e47d7
Cr-Commit-Position: refs/heads/master@{#431726}
Patch Set 1 #Patch Set 2 : test #
Total comments: 2
Patch Set 3 : Missing file #
Messages
Total messages: 28 (17 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...
Description was changed from ========== [Extensions] Don't show the pending URL for chrome.tabs API navigations For the pdf extension, treat navigations as renderer-initiated. Ideally, we want this to become default for all extensions, but that risks breakage. BUG=660498 TEST=See bug for repro steps. ========== to ========== [Extensions] Don't show the pending URL for chrome.tabs API navigations For the pdf extension, treat navigations as renderer-initiated. Ideally, we want this to become default for all extensions, but that risks breakage. Based on creis@'s patch at https://codereview.chromium.org/2475033002/. BUG=660498 TEST=See bug for repro steps. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rdevlin.cronin@chromium.org changed reviewers: + nasko@chromium.org
Nasko, mind taking a look? cc Charlie as FYI.
The code looks good, however it will be nice to have a test case for this. Is there a reason you didn't include one?
creis@chromium.org changed reviewers: + creis@chromium.org
LGTM, though I agree a test would be worth adding if possible. lfg@ or ekaramad@, do you have a pointer for where the PDF tests are? Maybe we could set up a TestNavigationManager in one of those tests to pause a navigation triggered by a PDF and make sure it doesn't change WebContents::GetVisibleURL()? (I'm open to other strategies as well.)
On 2016/11/11 18:26:12, Charlie Reis (OOO til 11-11) wrote: > LGTM, though I agree a test would be worth adding if possible. > > lfg@ or ekaramad@, do you have a pointer for where the PDF tests are? Maybe we > could set up a TestNavigationManager in one of those tests to pause a navigation > triggered by a PDF and make sure it doesn't change WebContents::GetVisibleURL()? > (I'm open to other strategies as well.) Can't we just have the PoC HTML file as part of the test data files, setup the TestNavigationManager for the target URL, then just navigate to the PoC html file and WaitForRequestStart on the navigation manager, which will stop it before commit. Then switch tabs around and check expectations?
On 2016/11/11 18:29:01, nasko wrote: > On 2016/11/11 18:26:12, Charlie Reis (OOO til 11-11) wrote: > > LGTM, though I agree a test would be worth adding if possible. > > > > lfg@ or ekaramad@, do you have a pointer for where the PDF tests are? Maybe > we > > could set up a TestNavigationManager in one of those tests to pause a > navigation > > triggered by a PDF and make sure it doesn't change > WebContents::GetVisibleURL()? > > (I'm open to other strategies as well.) > > Can't we just have the PoC HTML file as part of the test data files, setup the > TestNavigationManager for the target URL, then just navigate to the PoC html > file and WaitForRequestStart on the navigation manager, which will stop it > before commit. Then switch tabs around and check expectations? The main test location for GuestView related tests is MimeHandlerViewTest which loads its own version of Extension not the kPdfExtensionId one AFAIK. The test I am adding in my own pdf-related CL (https://codereview.chromium.org/2417693002/) is in 'chrome_site_per_process_browsertest.cc' where I had to manually create some GuestView related classes. If nasko@'s approach does not work, and in case we need to pause at specific times during the navigation, we could possibly inject code into guest WebContents (extension) to replace window.onmessage() and Navigator.navigateInCurrentTab properly, i.e., send message to browser before chrome.tabs.update() call and then provide the callback to it so that we know when it ended according to renderer.
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...
BrowserTest or UnitTest? https://i.ytimg.com/vi/X9Jl2I-hwfE/hqdefault.jpg Done.
Hooray! LGTM with one question. https://codereview.chromium.org/2492863003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2492863003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:2132: "/extensions/api_test/tabs/pdf_extension_test.html"); Do you need to add this file? I don't see it in the repo.
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...
https://codereview.chromium.org/2492863003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2492863003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:2132: "/extensions/api_test/tabs/pdf_extension_test.html"); On 2016/11/11 23:26:07, Charlie Reis (OOO til 11-11) wrote: > Do you need to add this file? I don't see it in the repo. Whoops! Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2492863003/#ps40001 (title: "Missing file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Don't show the pending URL for chrome.tabs API navigations For the pdf extension, treat navigations as renderer-initiated. Ideally, we want this to become default for all extensions, but that risks breakage. Based on creis@'s patch at https://codereview.chromium.org/2475033002/. BUG=660498 TEST=See bug for repro steps. ========== to ========== [Extensions] Don't show the pending URL for chrome.tabs API navigations For the pdf extension, treat navigations as renderer-initiated. Ideally, we want this to become default for all extensions, but that risks breakage. Based on creis@'s patch at https://codereview.chromium.org/2475033002/. BUG=660498 TEST=See bug for repro steps. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Don't show the pending URL for chrome.tabs API navigations For the pdf extension, treat navigations as renderer-initiated. Ideally, we want this to become default for all extensions, but that risks breakage. Based on creis@'s patch at https://codereview.chromium.org/2475033002/. BUG=660498 TEST=See bug for repro steps. ========== to ========== [Extensions] Don't show the pending URL for chrome.tabs API navigations For the pdf extension, treat navigations as renderer-initiated. Ideally, we want this to become default for all extensions, but that risks breakage. Based on creis@'s patch at https://codereview.chromium.org/2475033002/. BUG=660498 TEST=See bug for repro steps. Committed: https://crrev.com/2097de33a1f3d04e78d93e9e2f16aad4b97e47d7 Cr-Commit-Position: refs/heads/master@{#431726} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2097de33a1f3d04e78d93e9e2f16aad4b97e47d7 Cr-Commit-Position: refs/heads/master@{#431726} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
