Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13516002:
Whitelist Instant processes for content settings (Closed)
|
Created:
7 years, 8 months ago by ckocagil Modified:
7 years, 8 months ago CC:
chromium-reviews, melevin, dhollowa+watch_chromium.org, dougw+watch_chromium.org, gideonwald, dominich, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
http://git.chromium.org/chromium/src.git@omniboxtest Visibility:
Public. |
DescriptionWhitelist Instant processes for content settings.
This patch only grants JS and Image permissions to Instant processes.
BUG=225758
TEST=InstantTest.ContentSettingsWhitelist
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194900
Patch Set 1 #Patch Set 2 : test #Patch Set 3 : instantwhitelist #Patch Set 4 : 4 #
Total comments: 2
Messages
Total messages: 27 (0 generated)
This should allow all Instant processes to be whitelisted for content settings.
On 2013/04/03 07:15:18, ckocagil wrote: > This should allow all Instant processes to be whitelisted for content settings. Looks good. How about a browser test?
On 2013/04/03 15:55:05, sreeram wrote: > On 2013/04/03 07:15:18, ckocagil wrote: > > This should allow all Instant processes to be whitelisted for content > settings. > > Looks good. How about a browser test? I added one.
No takers?
LGTM. Thanks for the test!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/13516002/4001
Failed to apply patch for chrome/browser/instant/instant_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/instant/instant_browsertest.cc
Hunk #1 FAILED at 18.
Hunk #2 FAILED at 1002.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/instant/instant_browsertest.cc.rej
Patch: chrome/browser/instant/instant_browsertest.cc
Index: chrome/browser/instant/instant_browsertest.cc
diff --git a/chrome/browser/instant/instant_browsertest.cc
b/chrome/browser/instant/instant_browsertest.cc
index
e04f5c2d96844cad2435abfc138e2f9f99e688eb..643256c5f676417c39b17e7f7309fed9e5760f09
100644
--- a/chrome/browser/instant/instant_browsertest.cc
+++ b/chrome/browser/instant/instant_browsertest.cc
@@ -18,6 +18,7 @@
#include "chrome/browser/ui/host_desktop.h"
#include "chrome/browser/ui/omnibox/omnibox_view.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/content_settings_types.h"
#include "chrome/common/url_constants.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/interactive_test_utils.h"
@@ -1002,3 +1003,35 @@ IN_PROC_BROWSER_TEST_F(InstantTest, ProcessIsolation) {
EXPECT_FALSE(instant_service->IsInstantProcess(
active_tab->GetRenderProcessHost()->GetID()));
}
+
+IN_PROC_BROWSER_TEST_F(InstantTest, ContentSettingsWhitelist) {
+ // Creates a 2x2 pixel image element and checks its height after it loads.
+ const char* kImageRenderScript =
+ "var testImage = document.createElement('img');"
+ "testImage.onload = function () {"
+ " domAutomationController.send(testImage.height > 0); };"
+ "testImage.src = 'data:image/png;base64,Qk1GAAAAAAAAADYAAAAoAAAAAgAAAAIA"
+ "AAABABgAAAAAAAAAAADEDgAAxA4AAAAAAAAAAAAAAAAAAAAAAP8AAAAAAAAA/w==';";
+
+ Profile* profile = browser()->profile();
+
+ // Block images through content settings.
+ profile->GetHostContentSettingsMap()->SetDefaultContentSetting(
+ CONTENT_SETTINGS_TYPE_IMAGES, CONTENT_SETTING_BLOCK);
+
+ ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
+ FocusOmniboxAndWaitForInstantSupport();
+
+ InstantService* instant_service =
+ InstantServiceFactory::GetForProfile(profile);
+ ASSERT_NE(static_cast<InstantService*>(NULL), instant_service);
+
+ // Make sure this is an Instant process.
+ content::WebContents* overlay = instant()->GetOverlayContents();
+ EXPECT_TRUE(instant_service->IsInstantProcess(
+ overlay->GetRenderProcessHost()->GetID()));
+
+ bool result;
+ ExecuteScriptAndExtractBool(overlay, kImageRenderScript, &result);
+ EXPECT_TRUE(result);
+}
instantwhitelist
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/13516002/13001
Presubmit check for 13516002-13001 failed and returned exit status 1.
INFO:root:Found 2 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/renderer/content_settings_observer.cc
jochen: content_settings_observer.cc
The setting you modified only grants script and image content settings (but not cookies, location, or others..) lgtm
Oh, the name and description of IsWhitelistedForContentSettings is misleading. sreeram, is it sufficient to whitelist instant pages for images and scripts only?
let me clarify: If it was whitelisting for anything else, I wouldn't have approved the cl :)
On 2013/04/08 20:28:52, ckocagil wrote: > Oh, the name and description of IsWhitelistedForContentSettings is misleading. > > sreeram, is it sufficient to whitelist instant pages for images and scripts > only? No, we'd want it to be whitelisted for cookies too (what does that mean? allows first-party cookies? third party cookies?). I don't anything about content settings, so some other things we need: session storage, local storage. Location would be nice to have too (though I think the page can deal with it if it's blocked).
On 2013/04/08 20:30:46, sreeram wrote: > On 2013/04/08 20:28:52, ckocagil wrote: > > Oh, the name and description of IsWhitelistedForContentSettings is misleading. > > > > sreeram, is it sufficient to whitelist instant pages for images and scripts > > only? > > No, we'd want it to be whitelisted for cookies too (what does that mean? allows > first-party cookies? third party cookies?). I don't anything about content > settings, so some other things we need: session storage, local storage. Location > would be nice to have too (though I think the page can deal with it if it's > blocked). Please contact the privacy first before adding capabilities to bypass cookie settings.
> Please contact the privacy first before adding capabilities to bypass cookie > settings. privacy *team*
sreeram: Did you read the privacy team's response at http://crbug.com/225758 ? Do you think we should land this as it is?
On 2013/04/11 13:43:09, ckocagil wrote: > sreeram: Did you read the privacy team's response at http://crbug.com/225758 ? > Do you think we should land this as it is? Yes. This is an improvement over the current state of affairs in that it allows images and scripts. So, let's land this. We'll figure out how to handle the other stuff (cookies, web storage, location, etc) separately. LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/13516002/13001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
https://codereview.chromium.org/13516002/diff/33001/chrome/renderer/content_s... File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/13516002/diff/33001/chrome/renderer/content_s... chrome/renderer/content_settings_observer.cc:324: if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kInstantProcess)) Sorry for being late in this party. Does this whitelisting only affect the "chrome://newtab" page? If so why is the whitelisting not handled in the HostContentSettingsMap?
https://codereview.chromium.org/13516002/diff/33001/chrome/renderer/content_s... File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/13516002/diff/33001/chrome/renderer/content_s... chrome/renderer/content_settings_observer.cc:324: if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kInstantProcess)) On 2013/04/17 19:24:38, markusheintz_ wrote: > Sorry for being late in this party. Does this whitelisting only affect the > "chrome://newtab" page? If so why is the whitelisting not handled in the > HostContentSettingsMap? No, it affects all Instant processes, including the Instant Extended search suggestions/results overlay.
markusheintz: I'm going to commit this if you have no objection.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/13516002/33001
Message was sent while issue was closed.
Change committed as 194900 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
