Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
300873002:
Removed NPAPI plugin infobar, with Finch trial gate (disabled by default). (Closed)
|
Created:
6 years, 7 months ago by Chris Thompson Modified:
6 years, 6 months ago CC:
chromium-reviews, stuartmorgan+watch_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRemoved NPAPI plugin infobar, with Finch trial gate (disabled by default).
This change is related to the new plugin page action UI https://codereview.chromium.org/319553008/
BUG=307323
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278549
Patch Set 1 #Patch Set 2 : Change infobar removal group test slightly #Patch Set 3 : Moved finch trial gate to renderer plugin creator #Patch Set 4 : Revert plugin_observer, fix name collision in trial code #Patch Set 5 : Add to renderer field trial list #Patch Set 6 : Don't block infobar if not NPAPI (so PPAPI goes through) #Patch Set 7 : Add browser_tests for infobar removal #
Total comments: 3
Patch Set 8 : Added back ASSERTs as EXPECTs for better test debug-ability. #
Total comments: 12
Patch Set 9 : Address review comments #
Total comments: 1
Patch Set 10 : Remove unnecessary comment #
Total comments: 11
Patch Set 11 : Move tests out of renderer, address various reviewer comments #
Total comments: 1
Patch Set 12 : Updated to have dependent upstream CL, switched how field trials are set #
Total comments: 2
Patch Set 13 : Cleaned up fieldtrial tests, re-added field trial list to renderer pre-initialization, made page ac… #Patch Set 14 : Changing upstream back to origin/master #Patch Set 15 : Rebased to include new blocking page action UI #
Total comments: 5
Messages
Total messages: 30 (0 generated)
Everything looks good except for 1 thing https://codereview.chromium.org/300873002/diff/120001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/300873002/diff/120001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:124: // ASSERT_TRUE(contents); Do we need this comment out here?
https://codereview.chromium.org/300873002/diff/120001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/300873002/diff/120001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:124: // ASSERT_TRUE(contents); On 2014/06/06 17:05:40, radhikabhar wrote: > Do we need this comment out here? Uncommented and changed to EXPECT_TRUE (since these are mostly useful for debugging a failed test, and the actual assertion we want to make is on whether the infobar was shown or not).
https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:808: // Check to see if old infobar should be displayed nit: we normally encourage people to use proper grammar (meaning, end with a ".") https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:78: // is over. my brain can't quite parse that comment, can you please clean it up? https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:79: // TODO(cthomp) Remove/simplify when Finch trial is completed Please file a bug for this (a removal bug), assign it to yourself as owner, and then put the crbug.com/XXXX link here https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:83: bool do_infobar_enabled_trial) maybe try naming something like |infobar_trial_enabled_|? https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:132: // Compare to number of infobars expected why not do EXPECT_EQ instead of returning like this? https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:168: IN_PROC_BROWSER_TEST_F(InfoBarDisabledBrowserTest, TestDisabled) { would be good for symmetry here -- InfobarEnabled, InfobarDisabled
https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:808: // Check to see if old infobar should be displayed On 2014/06/06 22:55:33, felt wrote: > nit: we normally encourage people to use proper grammar (meaning, end with a > ".") Fixed. https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:78: // is over. On 2014/06/06 22:55:33, felt wrote: > my brain can't quite parse that comment, can you please clean it up? Agreed. Deleted and rewritten to explain the two cases we want to test. https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:79: // TODO(cthomp) Remove/simplify when Finch trial is completed On 2014/06/06 22:55:33, felt wrote: > Please file a bug for this (a removal bug), assign it to yourself as owner, and > then put the crbug.com/XXXX link here Updated comment, linking to crbug.com/381944 https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:83: bool do_infobar_enabled_trial) On 2014/06/06 22:55:33, felt wrote: > maybe try naming something like |infobar_trial_enabled_|? Done. https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:132: // Compare to number of infobars expected On 2014/06/06 22:55:33, felt wrote: > why not do EXPECT_EQ instead of returning like this? No particular reason. Changed to ASSERT_EQ inside method instead of returning. https://codereview.chromium.org/300873002/diff/140001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:168: IN_PROC_BROWSER_TEST_F(InfoBarDisabledBrowserTest, TestDisabled) { On 2014/06/06 22:55:34, felt wrote: > would be good for symmetry here -- InfobarEnabled, InfobarDisabled Missed that one. Fixed.
one last question, why is NOTRY=true set in the description? that turns off trybots when it goes through the CQ, and this CL needs trybots turned on to land https://codereview.chromium.org/300873002/diff/160001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/300873002/diff/160001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:132: // Compare to the number of infobars expected. nit: you don't need that comment now, since it should be obvious what ASSERT_EQ is comparing here
On 2014/06/06 23:58:12, felt wrote: > one last question, why is NOTRY=true set in the description? that turns off > trybots when it goes through the CQ, and this CL needs trybots turned on to land Removed. Was originally to prevent spurious use of bots while working on this CL. > https://codereview.chromium.org/300873002/diff/160001/chrome/renderer/chrome_... > File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): > > https://codereview.chromium.org/300873002/diff/160001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client_browsertest.cc:132: // Compare to > the number of infobars expected. > nit: you don't need that comment now, since it should be obvious what ASSERT_EQ > is comparing here Fixed.
lgtm, can you ask bauerb@ to review since he's the last one to touch this code? FYI the phrase "Blocking on new blocking UI CL" is confusing in the description, you might want to re-word
FTR, it's okay to just add new reviewers if you want to have them look at the code. https://codereview.chromium.org/300873002/diff/120001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/300873002/diff/120001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:124: // ASSERT_TRUE(contents); On 2014/06/06 17:37:48, Chris Thompson wrote: > On 2014/06/06 17:05:40, radhikabhar wrote: > > Do we need this comment out here? > > Uncommented and changed to EXPECT_TRUE (since these are mostly useful for > debugging a failed test, and the actual assertion we want to make is on whether > the infobar was shown or not). FTR, ASSERT_ and EXPECT_ are not used to distinguish between the actual assertion of the test and something else. ASSERT_ is for "fatal test failures", which mean that the test can't possibly continue, so it will return from the current method (which is why you can only use it in void methods). EXPECT_ is for regular test failures from which you can recover. https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/DEPS File chrome/renderer/DEPS (right): https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/DEPS#ne... chrome/renderer/DEPS:18: "+chrome/browser/infobars", This is not going to fly. Renderer code can't depend on browser code. You probably want to move the whole test to chrome/browser. https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:809: const std::string trial_group = Nit: We don't usually use const on local variables. https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:811: if (plugin.type != content::WebPluginInfo::PLUGIN_TYPE_NPAPI || Why are you doing this in the renderer instead of the browser? Also, are we going to do nothing now when an unauthorized plugin is blocked? https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:79: // TODO(cthomp) Remove/simplify when Finch trial is completed crbug.com/381944 Nit: The recommended format for TODOs is TODO(ldap): (colon after the closing parenthesis). https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:84: : infobar_trial_enabled_(infobar_trial_enabled) {} Nit: indent two more spaces. https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:88: command_line->AppendSwitchASCII(switches::kForceFieldTrials, You can enable field trials directly with FieldTrialList::CreateTrialsFromString (and the same string argument as on the command line), then you can enable them whenever you want. https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:105: content::RunMessageLoop(); Prefer a MessageLoopRunner with an explicit quit closure that you can pass to PostTaskAndReply above. https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:114: } else { `else` isn't necessary if the `if` branch returns. You could also simplify this to to just return base::PathExists(...); https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:119: void InfoBarCountTest(unsigned int number_infobars_expected, Don't use `unsigned int`, use a regular int or a specific type. In this case, you want to compare the size of a container, so use size_t. https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:124: EXPECT_TRUE(contents != NULL); If contents is actually NULL, you will crash below. If you want to handle that case graceful, you can ASSERT. Also, the comparison to NULL is unnecessary. https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_browsertest.cc:140: class InfoBarEnabledBrowserTest : Do you actually need these two test fixtures? Just set |infobar_trial_enabled_| when you need to.
On 2014/06/09 17:03:19, Bernhard Bauer wrote: > FTR, it's okay to just add new reviewers if you want to have them look at the > code. > > https://codereview.chromium.org/300873002/diff/120001/chrome/renderer/chrome_... > File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): > > https://codereview.chromium.org/300873002/diff/120001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client_browsertest.cc:124: // > ASSERT_TRUE(contents); > On 2014/06/06 17:37:48, Chris Thompson wrote: > > On 2014/06/06 17:05:40, radhikabhar wrote: > > > Do we need this comment out here? > > > > Uncommented and changed to EXPECT_TRUE (since these are mostly useful for > > debugging a failed test, and the actual assertion we want to make is on > whether > > the infobar was shown or not). > > FTR, ASSERT_ and EXPECT_ are not used to distinguish between the actual > assertion of the test and something else. ASSERT_ is for "fatal test failures", > which mean that the test can't possibly continue, so it will return from the > current method (which is why you can only use it in void methods). EXPECT_ is > for regular test failures from which you can recover. > Thanks for the clarification. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/DEPS > File chrome/renderer/DEPS (right): > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/DEPS#ne... > chrome/renderer/DEPS:18: "+chrome/browser/infobars", > This is not going to fly. Renderer code can't depend on browser code. You > probably want to move the whole test to chrome/browser. > Moved the tests to chrome/browser/npapi_infobar_browsertests.cc, removed dependencies. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > File chrome/renderer/chrome_content_renderer_client.cc (right): > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client.cc:809: const std::string > trial_group = > Nit: We don't usually use const on local variables. > Removed const. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client.cc:811: if (plugin.type != > content::WebPluginInfo::PLUGIN_TYPE_NPAPI || > Why are you doing this in the renderer instead of the browser? > This is where two different code paths are dispatched: (1) kUnauthorized triggers a render_frame IPC that creates an infobar (2) kBlocked (and others) trigger a observer->DidBlockContentType notification that creates the page action This avoids duplicating the code path that creates the page action in the receiver of the message in (1). However, I'm open to better ideas for refactoring this change. > Also, are we going to do nothing now when an unauthorized plugin is blocked? > The CL referenced in the description will add the new UI (a page action). That will use the code path in the kBlocked case (see chrome/renderer/chrome_content_renderer_client.cc:836). The current render_frame IPC will be triggered if it isn't NPAPI, and the page-action observer->DidBlockContentType will be triggered for NPAPI plugins. I'll be merging the changes from that CL into this one before setting the commit bit on this one. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client_browsertest.cc:79: // > TODO(cthomp) Remove/simplify when Finch trial is completed crbug.com/381944 > Nit: The recommended format for TODOs is TODO(ldap): (colon after the closing > parenthesis). > Fixed. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client_browsertest.cc:84: : > infobar_trial_enabled_(infobar_trial_enabled) {} > Nit: indent two more spaces. > Fixed. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client_browsertest.cc:88: > command_line->AppendSwitchASCII(switches::kForceFieldTrials, > You can enable field trials directly with FieldTrialList::CreateTrialsFromString > (and the same string argument as on the command line), then you can enable them > whenever you want. > I tried switching the test class to use this, but couldn't get it to work (issues with the singleton not being set up yet I think). All of the existing Finch trial tests just add flags during command line setup. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client_browsertest.cc:105: > content::RunMessageLoop(); > Prefer a MessageLoopRunner with an explicit quit closure that you can pass to > PostTaskAndReply above. > Alright. I copied this test framework from https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pol..., but I implemented your preference here. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client_browsertest.cc:114: } else { > `else` isn't necessary if the `if` branch returns. > > You could also simplify this to to just return base::PathExists(...); > Fixed. Went with the latter approach -- much cleaner. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client_browsertest.cc:119: void > InfoBarCountTest(unsigned int number_infobars_expected, > Don't use `unsigned int`, use a regular int or a specific type. In this case, > you want to compare the size of a container, so use size_t. > Fixed. Meant to use size_t. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client_browsertest.cc:124: > EXPECT_TRUE(contents != NULL); > If contents is actually NULL, you will crash below. If you want to handle that > case graceful, you can ASSERT. Also, the comparison to NULL is unnecessary. > Fixed. Changed to ASSERT_TRUE, removed explicit comparison. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client_browsertest.cc:140: class > InfoBarEnabledBrowserTest : > Do you actually need these two test fixtures? Just set |infobar_trial_enabled_| > when you need to. Simplified the tests to just build off the main class instead of subclassing.
Also FYI, you can reply to specific comments by clicking on the link, which will group your reply together with the initial comment and allow you to work on them independently. On 2014/06/10 02:29:13, Chris Thompson wrote: > On 2014/06/09 17:03:19, Bernhard Bauer wrote: > > FTR, it's okay to just add new reviewers if you want to have them look at the > > code. > > > > > https://codereview.chromium.org/300873002/diff/120001/chrome/renderer/chrome_... > > File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): > > > > > https://codereview.chromium.org/300873002/diff/120001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client_browsertest.cc:124: // > > ASSERT_TRUE(contents); > > On 2014/06/06 17:37:48, Chris Thompson wrote: > > > On 2014/06/06 17:05:40, radhikabhar wrote: > > > > Do we need this comment out here? > > > > > > Uncommented and changed to EXPECT_TRUE (since these are mostly useful for > > > debugging a failed test, and the actual assertion we want to make is on > > whether > > > the infobar was shown or not). > > > > FTR, ASSERT_ and EXPECT_ are not used to distinguish between the actual > > assertion of the test and something else. ASSERT_ is for "fatal test > failures", > > which mean that the test can't possibly continue, so it will return from the > > current method (which is why you can only use it in void methods). EXPECT_ is > > for regular test failures from which you can recover. > > > > Thanks for the clarification. > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/DEPS > > File chrome/renderer/DEPS (right): > > > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/DEPS#ne... > > chrome/renderer/DEPS:18: "+chrome/browser/infobars", > > This is not going to fly. Renderer code can't depend on browser code. You > > probably want to move the whole test to chrome/browser. > > > > Moved the tests to chrome/browser/npapi_infobar_browsertests.cc, removed > dependencies. > > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > File chrome/renderer/chrome_content_renderer_client.cc (right): > > > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client.cc:809: const std::string > > trial_group = > > Nit: We don't usually use const on local variables. > > > > Removed const. > > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client.cc:811: if (plugin.type != > > content::WebPluginInfo::PLUGIN_TYPE_NPAPI || > > Why are you doing this in the renderer instead of the browser? > > > > This is where two different code paths are dispatched: > > (1) kUnauthorized triggers a render_frame IPC that creates an infobar > > (2) kBlocked (and others) trigger a observer->DidBlockContentType notification > that creates the page action > > > > This avoids duplicating the code path that creates the page action in the > receiver of the message in (1). However, I'm open to better ideas for > refactoring this change. > > > Also, are we going to do nothing now when an unauthorized plugin is blocked? > > > > The CL referenced in the description will add the new UI (a page action). That > will use the code path in the kBlocked case (see > chrome/renderer/chrome_content_renderer_client.cc:836). The current render_frame > IPC will be triggered if it isn't NPAPI, and the page-action > observer->DidBlockContentType will be triggered for NPAPI plugins. Oh, I see, we're going to call observer->DidBlockContentType instead. I would probably create a local branch that contains the changes from https://codereview.chromium.org/319553008/, merge that in here, and then set that branch as upstream, so `git cl upload` will upload the diff to that branch. Then when the other CL has landed, merge with origin/master and set upstream back to origin/master. That way the diff context will always be right, you don't have to deal with merge conflicts later, and the reviewer can see the final version. > I'll be merging the changes from that CL into this one before setting the commit > bit on this one. > > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): > > > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client_browsertest.cc:79: // > > TODO(cthomp) Remove/simplify when Finch trial is completed crbug.com/381944 > > Nit: The recommended format for TODOs is TODO(ldap): (colon after the closing > > parenthesis). > > > > Fixed. > > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client_browsertest.cc:84: : > > infobar_trial_enabled_(infobar_trial_enabled) {} > > Nit: indent two more spaces. > > > > Fixed. > > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client_browsertest.cc:88: > > command_line->AppendSwitchASCII(switches::kForceFieldTrials, > > You can enable field trials directly with > FieldTrialList::CreateTrialsFromString > > (and the same string argument as on the command line), then you can enable > them > > whenever you want. > > > > I tried switching the test class to use this, but couldn't get it to work > (issues with the singleton not being set up yet I think). All of the existing > Finch trial tests just add flags during command line setup. Hm, I don't think this will work now though, because SetUpCommandLine runs before you set |infobar_trial_enabled_|. I think it would be worth putting some effort into finding out why FieldTrialList::CreateTrialsFromString isn't working, because setting command line flags really is kinda clunky. > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client_browsertest.cc:105: > > content::RunMessageLoop(); > > Prefer a MessageLoopRunner with an explicit quit closure that you can pass to > > PostTaskAndReply above. > > > > Alright. I copied this test framework from > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pol..., > but I implemented your preference here. I would not be surprised if that simply predates MessageLoopRunner &c. > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client_browsertest.cc:114: } else { > > `else` isn't necessary if the `if` branch returns. > > > > You could also simplify this to to just return base::PathExists(...); > > > > Fixed. Went with the latter approach -- much cleaner. > > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client_browsertest.cc:119: void > > InfoBarCountTest(unsigned int number_infobars_expected, > > Don't use `unsigned int`, use a regular int or a specific type. In this case, > > you want to compare the size of a container, so use size_t. > > > > Fixed. Meant to use size_t. > > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client_browsertest.cc:124: > > EXPECT_TRUE(contents != NULL); > > If contents is actually NULL, you will crash below. If you want to handle that > > case graceful, you can ASSERT. Also, the comparison to NULL is unnecessary. > > > > Fixed. Changed to ASSERT_TRUE, removed explicit comparison. > > > > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client_browsertest.cc:140: class > > InfoBarEnabledBrowserTest : > > Do you actually need these two test fixtures? Just set > |infobar_trial_enabled_| > > when you need to. > > Simplified the tests to just build off the main class instead of subclassing. https://codereview.chromium.org/300873002/diff/200001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/300873002/diff/200001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:78: 'browser/npapi_infobar_browsertest.cc', Maybe move this into browser/plugins/?
On 2014/06/10 08:53:57, Bernhard Bauer wrote: > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > > chrome/renderer/chrome_content_renderer_client.cc:811: if (plugin.type != > > > content::WebPluginInfo::PLUGIN_TYPE_NPAPI || > > > Why are you doing this in the renderer instead of the browser? > > > > > > > This is where two different code paths are dispatched: > > > > > (1) kUnauthorized triggers a render_frame IPC that creates an infobar > > > > > (2) kBlocked (and others) trigger a observer->DidBlockContentType notification > > that creates the page action > > > > > > > > > > This avoids duplicating the code path that creates the page action in the > > receiver of the message in (1). However, I'm open to better ideas for > > refactoring this change. > > > > > Also, are we going to do nothing now when an unauthorized plugin is blocked? > > > > > > > The CL referenced in the description will add the new UI (a page action). That > > will use the code path in the kBlocked case (see > > chrome/renderer/chrome_content_renderer_client.cc:836). The current > render_frame > > IPC will be triggered if it isn't NPAPI, and the page-action > > observer->DidBlockContentType will be triggered for NPAPI plugins. > > > Oh, I see, we're going to call observer->DidBlockContentType instead. > > I would probably create a local branch that contains the changes from > https://codereview.chromium.org/319553008/, merge that in here, and then set > that branch as upstream, so `git cl upload` will upload the diff to that branch. > Then when the other CL has landed, merge with origin/master and set upstream > back to origin/master. That way the diff context will always be right, you don't > have to deal with merge conflicts later, and the reviewer can see the final > version. > Done. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > > chrome/renderer/chrome_content_renderer_client_browsertest.cc:88: > > > command_line->AppendSwitchASCII(switches::kForceFieldTrials, > > > You can enable field trials directly with > > FieldTrialList::CreateTrialsFromString > > > (and the same string argument as on the command line), then you can enable > > them > > > whenever you want. > > > > > > > I tried switching the test class to use this, but couldn't get it to work > > (issues with the singleton not being set up yet I think). All of the existing > > Finch trial tests just add flags during command line setup. > > Hm, I don't think this will work now though, because SetUpCommandLine runs > before you set |infobar_trial_enabled_|. > > I think it would be worth putting some effort into finding out why > FieldTrialList::CreateTrialsFromString isn't working, because setting command > line flags really is kinda clunky. Figured out how to get this working, and got rid of the SetupCommandLine stuff. > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > > chrome/renderer/chrome_content_renderer_client_browsertest.cc:140: class > > > InfoBarEnabledBrowserTest : > > > Do you actually need these two test fixtures? Just set > > |infobar_trial_enabled_| > > > when you need to. > > > > Simplified the tests to just build off the main class instead of subclassing. > > https://codereview.chromium.org/300873002/diff/200001/chrome/chrome_tests.gypi > File chrome/chrome_tests.gypi (right): > > https://codereview.chromium.org/300873002/diff/200001/chrome/chrome_tests.gyp... > chrome/chrome_tests.gypi:78: 'browser/npapi_infobar_browsertest.cc', > Maybe move this into browser/plugins/? Done.
LGTM! On 2014/06/10 22:49:48, Chris Thompson wrote: > https://codereview.chromium.org/300873002/diff/180001/chrome/renderer/chrome_... > > > > chrome/renderer/chrome_content_renderer_client_browsertest.cc:88: > > > > command_line->AppendSwitchASCII(switches::kForceFieldTrials, > > > > You can enable field trials directly with > > > FieldTrialList::CreateTrialsFromString > > > > (and the same string argument as on the command line), then you can enable > > > them > > > > whenever you want. > > > > > > > > > > I tried switching the test class to use this, but couldn't get it to work > > > (issues with the singleton not being set up yet I think). All of the > existing > > > Finch trial tests just add flags during command line setup. > > > > Hm, I don't think this will work now though, because SetUpCommandLine runs > > before you set |infobar_trial_enabled_|. > > > > I think it would be worth putting some effort into finding out why > > FieldTrialList::CreateTrialsFromString isn't working, because setting command > > line flags really is kinda clunky. > > Figured out how to get this working, and got rid of the SetupCommandLine stuff. Awesome, thanks! https://codereview.chromium.org/300873002/diff/220001/chrome/browser/plugins/... File chrome/browser/plugins/npapi_infobar_browsertest.cc (right): https://codereview.chromium.org/300873002/diff/220001/chrome/browser/plugins/... chrome/browser/plugins/npapi_infobar_browsertest.cc:90: base::FieldTrialList field_trial_list(NULL); I think you could make this a member?
https://codereview.chromium.org/300873002/diff/220001/chrome/browser/plugins/... File chrome/browser/plugins/npapi_infobar_browsertest.cc (right): https://codereview.chromium.org/300873002/diff/220001/chrome/browser/plugins/... chrome/browser/plugins/npapi_infobar_browsertest.cc:90: base::FieldTrialList field_trial_list(NULL); On 2014/06/11 08:37:28, Bernhard Bauer wrote: > I think you could make this a member? Turns out I don't need to initialize it at all. Not 100% sure where my previous issues were coming from, but just calling CreateTrialsFromString is working now (no member needed, and local var deleted).
The CQ bit was checked by cthomp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cthomp@chromium.org/300873002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
sky@ could you PTAL for OWNERS approval? Thanks.
On 2014/06/19 19:51:47, Chris Thompson wrote: > sky@ could you PTAL for OWNERS approval? Thanks. Remind me what files I need to review.
On 2014/06/19 20:31:07, sky wrote: > On 2014/06/19 19:51:47, Chris Thompson wrote: > > sky@ could you PTAL for OWNERS approval? Thanks. > > Remind me what files I need to review. chrome/renderer/chrome_content_renderer_client.cc and chrome/chrome_tests.gpyi. Also, adding asvitkine@ -- could you PTAL at chrome/browser/chrome_browser_field_trials.cc?
https://codereview.chromium.org/300873002/diff/280001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/300873002/diff/280001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials.cc:64: base::FieldTrialList::FindValue("UnauthorizedPluginInfoBar"); This is no longer needed as you can specify "starts_active":true in the server-side config to get the same effect.
On 2014/06/19 20:38:22, Alexei Svitkine wrote: > https://codereview.chromium.org/300873002/diff/280001/chrome/browser/chrome_b... > File chrome/browser/chrome_browser_field_trials.cc (right): > > https://codereview.chromium.org/300873002/diff/280001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_field_trials.cc:64: > base::FieldTrialList::FindValue("UnauthorizedPluginInfoBar"); > This is no longer needed as you can specify "starts_active":true in the > server-side config to get the same effect. I was unable to manually test the trial using the "--force-fieldtrials" command-line flag without this. However, now that I've finished manual testing and have a browsertest that activates them, I agree it might be reasonable to remove. Thoughts?
lgtm On 2014/06/19 20:40:26, Chris Thompson wrote: > On 2014/06/19 20:38:22, Alexei Svitkine wrote: > > > https://codereview.chromium.org/300873002/diff/280001/chrome/browser/chrome_b... > > File chrome/browser/chrome_browser_field_trials.cc (right): > > > > > https://codereview.chromium.org/300873002/diff/280001/chrome/browser/chrome_b... > > chrome/browser/chrome_browser_field_trials.cc:64: > > base::FieldTrialList::FindValue("UnauthorizedPluginInfoBar"); > > This is no longer needed as you can specify "starts_active":true in the > > server-side config to get the same effect. > > I was unable to manually test the trial using the "--force-fieldtrials" > command-line flag without this. However, now that I've finished manual testing > and have a browsertest that activates them, I agree it might be reasonable to > remove. Thoughts? Up to you. Once you haver a server-side config, you can test with --force-fieldtrials. But if you want to be able to test outside of that, I'm okay with you keeping that line so that the command-line works.
LGTM
The CQ bit was checked by cthomp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cthomp@chromium.org/300873002/280001
Message was sent while issue was closed.
Change committed as 278549
Message was sent while issue was closed.
https://codereview.chromium.org/300873002/diff/280001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/300873002/diff/280001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1301: 'browser/plugins/npapi_infobar_browsertest.cc', Wait, why is this in here twice now? Also, there's a tab character.
Message was sent while issue was closed.
https://codereview.chromium.org/300873002/diff/280001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/300873002/diff/280001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1301: 'browser/plugins/npapi_infobar_browsertest.cc', On 2014/06/20 10:43:40, Bernhard Bauer wrote: > Wait, why is this in here twice now? > > Also, there's a tab character. and This should be alphabetically ordered.
Message was sent while issue was closed.
I've made a new CL to fix the duplicate include and these style mistakes: https://codereview.chromium.org/345173002 (and set sky@ as reviewer for it again, but if any of you want to double check it feel free). https://codereview.chromium.org/300873002/diff/280001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/300873002/diff/280001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1301: 'browser/plugins/npapi_infobar_browsertest.cc', On 2014/06/20 10:43:40, Bernhard Bauer wrote: > Wait, why is this in here twice now? > > Also, there's a tab character. My mistake when adding them to the build. This should only be a normal browser_test. And apparently my Vim config doesn't understand JSON. https://codereview.chromium.org/300873002/diff/280001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1301: 'browser/plugins/npapi_infobar_browsertest.cc', On 2014/06/20 14:10:16, limasdf wrote: > On 2014/06/20 10:43:40, Bernhard Bauer wrote: > > Wait, why is this in here twice now? > > > > Also, there's a tab character. > > and This should be alphabetically ordered. Forgot to move it when I changed the parent folder for the test. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
