Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
216113007:
Check the scheme of a RenderViewHost before assuming it contains an extension. (Closed)
|
Created:
6 years, 9 months ago by Jeffrey Yasskin Modified:
6 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCheck the scheme of a RenderViewHost before assuming it contains an extension.
Thanks to ncarter for the test!
BUG=357382
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261496
Patch Set 1 #
Total comments: 2
Patch Set 2 : scheme() constructs a string instead of a StringPiece. Who knew? #Patch Set 3 : Adopt Nick's test and fix sense of scheme check. #
Total comments: 2
Patch Set 4 : Allow the chrome-guest: scheme to fix WebViewTest.ContextMenusAPI_Basic #Patch Set 5 : Sync to r260786 #
Messages
Total messages: 29 (0 generated)
Here's a fix. Nick, if you have time to write a test, that'd be greatly appreciated.
I'll take a stab at a test right now; I have an idea. https://codereview.chromium.org/216113007/diff/1/extensions/browser/process_m... File extensions/browser/process_manager.cc (right): https://codereview.chromium.org/216113007/diff/1/extensions/browser/process_m... extensions/browser/process_manager.cc:67: if (site_instance->GetSiteURL().scheme() != kExtensionScheme) Comments in GURL seem to suggest that .SchemeIs(kExtensionScheme) would be the preferred idiom.
https://codereview.chromium.org/216113007/diff/1/extensions/browser/process_m... File extensions/browser/process_manager.cc (right): https://codereview.chromium.org/216113007/diff/1/extensions/browser/process_m... extensions/browser/process_manager.cc:67: if (site_instance->GetSiteURL().scheme() != kExtensionScheme) On 2014/03/28 19:25:31, ncarter wrote: > Comments in GURL seem to suggest that .SchemeIs(kExtensionScheme) would be the > preferred idiom. Indeed, it's much better. Thanks.
I've got a test up here. Should we land separately, or merge together?
https://codereview.chromium.org/216113007/diff/40001/chrome/browser/extension... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/216113007/diff/40001/chrome/browser/extension... chrome/browser/extensions/process_manager_browsertest.cc:151: ASSERT_EQ(1u, pm->GetRenderViewHostsForExtension(extension->id()).size()); I made this an ASSERT so the next line won't crash if it fails.
lgtm https://codereview.chromium.org/216113007/diff/40001/chrome/browser/extension... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/216113007/diff/40001/chrome/browser/extension... chrome/browser/extensions/process_manager_browsertest.cc:151: ASSERT_EQ(1u, pm->GetRenderViewHostsForExtension(extension->id()).size()); On 2014/03/31 23:15:40, Jeffrey Yasskin wrote: > I made this an ASSERT so the next line won't crash if it fails. Great!
The CQ bit was checked by jyasskin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jyasskin@chromium.org/216113007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by jyasskin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jyasskin@chromium.org/216113007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
The CQ bit was checked by jyasskin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jyasskin@chromium.org/216113007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
Fady, does it make sense to treat a chrome-guest: WebContents as an Extension view? I had to allow it to fix WebViewTest.ContextMenusAPI_Basic in http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu..., but it seems sketchy.
fady: ping
lgtm
The CQ bit was checked by jyasskin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jyasskin@chromium.org/216113007/70001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) telemetry_perf_unittests, telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by jyasskin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jyasskin@chromium.org/216113007/70001
Message was sent while issue was closed.
Change committed as 261496 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
