Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
319553008:
Updated Plugin bubble model to add "learn more" link and to appear with a sliding yellow thing (Closed)
|
Created:
6 years, 6 months ago by radhikabhar Modified:
6 years, 6 months ago CC:
chromium-reviews, tfarina, markusheintz_ Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUpdated Plugin bubble model to add "learn more" link and to appear with a sliding yellow thing.
BUG=307323
This change is linked to https://codereview.chromium.org/300873002/
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278302
Patch Set 1 : Changelist for the default radio button #Patch Set 2 : Updated NPAPI plugin including the default radio button settings #Patch Set 3 : Fixed indentation and style issues #
Total comments: 1
Patch Set 4 : Fixed the crash when javascript is disabled and added Test Cases #Patch Set 5 : Fixed typos #
Total comments: 14
Patch Set 6 : Addressed reviewer comments and fixed style issues #Patch Set 7 : Remove an extra declared function #
Total comments: 14
Patch Set 8 : Fixed various comments #
Total comments: 2
Patch Set 9 : Changed the comment line #
Total comments: 8
Patch Set 10 : Fixed comments #
Total comments: 30
Patch Set 11 : Addressed various comments #Patch Set 12 : Reverted to the old UI apart from the Learn More Link #Patch Set 13 : Fixed compilation errors in try bots for chromium_chromeos_rel #Patch Set 14 : Updated so that the yellow thing occurs only when the content setting is set to allow #Messages
Total messages: 45 (0 generated)
Overall looks good except for some style nits https://codereview.chromium.org/319553008/diff/100001/chrome/browser/content_... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/319553008/diff/100001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:264: plugin_bubble_setting_ = setting; over-indented https://codereview.chromium.org/319553008/diff/100001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/319553008/diff/100001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:259: set_plugin_bubble_setting(setting); Check style for this indentation. https://codereview.chromium.org/319553008/diff/100001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:269: return selected_item_ != bubble_content().radio_group.default_item; over-indented https://codereview.chromium.org/319553008/diff/100001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:518: OnCustomLinkClicked(); over-indented (by 1 space) https://codereview.chromium.org/319553008/diff/100001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc (right): https://codereview.chromium.org/319553008/diff/100001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:468: EXPECT_EQ(CONTENT_SETTING_ALLOW, over-indented (by 1 space)
On 2014/06/06 16:52:06, Chris Thompson wrote: > Overall looks good except for some style nits > > https://codereview.chromium.org/319553008/diff/100001/chrome/browser/content_... > File chrome/browser/content_settings/tab_specific_content_settings.h (right): > > https://codereview.chromium.org/319553008/diff/100001/chrome/browser/content_... > chrome/browser/content_settings/tab_specific_content_settings.h:264: > plugin_bubble_setting_ = setting; > over-indented > > https://codereview.chromium.org/319553008/diff/100001/chrome/browser/ui/conte... > File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): > > https://codereview.chromium.org/319553008/diff/100001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:259: > set_plugin_bubble_setting(setting); > Check style for this indentation. > > https://codereview.chromium.org/319553008/diff/100001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:269: return > selected_item_ != bubble_content().radio_group.default_item; > over-indented > > https://codereview.chromium.org/319553008/diff/100001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:518: > OnCustomLinkClicked(); > over-indented (by 1 space) > > https://codereview.chromium.org/319553008/diff/100001/chrome/browser/ui/conte... > File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc > (right): > > https://codereview.chromium.org/319553008/diff/100001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:468: > EXPECT_EQ(CONTENT_SETTING_ALLOW, > over-indented (by 1 space) Addressed the comments from Chris
can you please update the description to explain everything that's going on in this CL? thanks!
On 2014/06/06 17:05:41, felt wrote: > can you please update the description to explain everything that's going on in > this CL? thanks! Updated the description
https://codereview.chromium.org/319553008/diff/120001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc (right): https://codereview.chromium.org/319553008/diff/120001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:463: // EXPECT_TRUE(bubble_content.custom_link_enabled); I think compilation is failing because the macro is getting expanded and breaking out of the comment.
Are these tests passing now? Here are some comments. https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:261: //Set whether the setting for the plugin bubble mode is Allow, Block or Run You need a space and a period at the end of the sentence, like: // Set whether ... // this time. https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:283: // TODO(radhikabhar): This is no longer needed as we have deleted the custom Why is this a TODO? Why aren't you doing it right here? https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:424: // custom link Same question, why is this a TODO? https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/brows... File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right): https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/brows... chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:54: switch (type) { Does this make sense as a switch, if there's only one case you handle? Should it be an if-statement? https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc (right): https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:462: // TODO(cthomp): We no longer use custom link for plugins, so remove test? Another TODO here, can you delete the commented out code? https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:466: //Test to see that the default radio item is set to Block Missing a space: // Test to ... https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:472: TabSpecificContentSettings::FromWebContents(web_contents()); indent line 472 by 4 more spaces
On 2014/06/11 02:37:30, felt wrote: > Are these tests passing now? > > Here are some comments. > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... > File chrome/browser/content_settings/tab_specific_content_settings.h (right): > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... > chrome/browser/content_settings/tab_specific_content_settings.h:261: //Set > whether the setting for the plugin bubble mode is Allow, Block or Run > You need a space and a period at the end of the sentence, like: > > // Set whether ... > // this time. > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... > chrome/browser/content_settings/tab_specific_content_settings.h:283: // > TODO(radhikabhar): This is no longer needed as we have deleted the custom > Why is this a TODO? Why aren't you doing it right here? > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... > chrome/browser/content_settings/tab_specific_content_settings.h:424: // custom > link > Same question, why is this a TODO? > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/brows... > File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right): > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/brows... > chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:54: switch > (type) { > Does this make sense as a switch, if there's only one case you handle? Should it > be an if-statement? > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... > File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc > (right): > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:462: > // TODO(cthomp): We no longer use custom link for plugins, so remove test? > Another TODO here, can you delete the commented out code? > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:466: > //Test to see that the default radio item is set to Block > Missing a space: > // Test to ... > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:472: > TabSpecificContentSettings::FromWebContents(web_contents()); > indent line 472 by 4 more spaces The tests are passing now. Deleted all the unnecessary and extraneous code.
On 2014/06/11 17:00:29, radhikabhar wrote: > On 2014/06/11 02:37:30, felt wrote: > > Are these tests passing now? > > > > Here are some comments. > > > > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... > > File chrome/browser/content_settings/tab_specific_content_settings.h (right): > > > > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... > > chrome/browser/content_settings/tab_specific_content_settings.h:261: //Set > > whether the setting for the plugin bubble mode is Allow, Block or Run > > You need a space and a period at the end of the sentence, like: > > > > // Set whether ... > > // this time. > > > > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... > > chrome/browser/content_settings/tab_specific_content_settings.h:283: // > > TODO(radhikabhar): This is no longer needed as we have deleted the custom > > Why is this a TODO? Why aren't you doing it right here? > > > > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... > > chrome/browser/content_settings/tab_specific_content_settings.h:424: // custom > > link > > Same question, why is this a TODO? > > > > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/brows... > > File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc > (right): > > > > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/brows... > > chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:54: switch > > (type) { > > Does this make sense as a switch, if there's only one case you handle? Should > it > > be an if-statement? > > > > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... > > File > chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc > > (right): > > > > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... > > > chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:462: > > // TODO(cthomp): We no longer use custom link for plugins, so remove test? > > Another TODO here, can you delete the commented out code? > > > > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... > > > chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:466: > > //Test to see that the default radio item is set to Block > > Missing a space: > > // Test to ... > > > > > https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... > > > chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:472: > > TabSpecificContentSettings::FromWebContents(web_contents()); > > indent line 472 by 4 more spaces > > The tests are passing now. Deleted all the unnecessary and extraneous code. During a code review, the normal way to handle comments is to go through each one and respond "Done" (or some other response) to each one individually. It makes it easier for the reviewer to keep track of which comments have been addressed. Can you please go through and do that?
Updated it as comments https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:261: //Set whether the setting for the plugin bubble mode is Allow, Block or Run On 2014/06/11 02:37:29, felt wrote: > You need a space and a period at the end of the sentence, like: > > // Set whether ... > // this time. Done. https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:283: // TODO(radhikabhar): This is no longer needed as we have deleted the custom On 2014/06/11 02:37:29, felt wrote: > Why is this a TODO? Why aren't you doing it right here? Removed it https://codereview.chromium.org/319553008/diff/300001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:424: // custom link On 2014/06/11 02:37:29, felt wrote: > Same question, why is this a TODO? Done. https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/brows... File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right): https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/brows... chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:54: switch (type) { On 2014/06/11 02:37:30, felt wrote: > Does this make sense as a switch, if there's only one case you handle? Should it > be an if-statement? Done. https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc (right): https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:462: // TODO(cthomp): We no longer use custom link for plugins, so remove test? On 2014/06/11 02:37:30, felt wrote: > Another TODO here, can you delete the commented out code? Done. https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:466: //Test to see that the default radio item is set to Block On 2014/06/11 02:37:30, felt wrote: > Missing a space: > // Test to ... Done. https://codereview.chromium.org/319553008/diff/300001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:472: TabSpecificContentSettings::FromWebContents(web_contents()); On 2014/06/11 02:37:30, felt wrote: > indent line 472 by 4 more spaces Done.
https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/brows... File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right): https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/brows... chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:54: if (type == CONTENT_SETTINGS_TYPE_PLUGINS) { cleaner to gate with an if-statement at the very beginning, like: if (type != CONTENT_SETTINGS_TYPE_PLUGINS) return; chrome::AddSelectedTabWithURL(... https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:167: if (learn_more_id) are there situations in which learn_more_id might not be set? should you add a DCHECK(learn_more_id)? https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:470: block_setting_(CONTENT_SETTING_BLOCK), indent lines 470-472 by 2 so that they line up with ContentSettingTitleAnd... https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:479: if (settings_changed()) { again here, simpler to do: if (!settings_changed()) return; if (content_type() != CONTENT_SETTINGS_TYPE_PLUGINS)) return; ... https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:480: ContentSetting setting; why is this outside of the if-statement, but ti's only used within the scope of the if-statement? https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:548: setting_is_wildcard) { ^ is that really over 80 chars? https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:553: } else if ( setting == CONTENT_SETTING_ALLOW) { get rid of the unneeded space
Updated various comments https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/brows... File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right): https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/brows... chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:54: if (type == CONTENT_SETTINGS_TYPE_PLUGINS) { On 2014/06/12 01:44:33, felt wrote: > cleaner to gate with an if-statement at the very beginning, like: > > if (type != CONTENT_SETTINGS_TYPE_PLUGINS) > return; > chrome::AddSelectedTabWithURL(... Done. https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:167: if (learn_more_id) On 2014/06/12 01:44:33, felt wrote: > are there situations in which learn_more_id might not be set? should you add a > DCHECK(learn_more_id)? DCHECK will crash chrome if we try to click on the bubble model for any content type apart from plugins as learn_more_id is not set for any other content type https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:470: block_setting_(CONTENT_SETTING_BLOCK), On 2014/06/12 01:44:33, felt wrote: > indent lines 470-472 by 2 so that they line up with ContentSettingTitleAnd... Done. https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:479: if (settings_changed()) { On 2014/06/12 01:44:33, felt wrote: > again here, simpler to do: > > if (!settings_changed()) > return; > if (content_type() != CONTENT_SETTINGS_TYPE_PLUGINS)) > return; > ... Done. https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:480: ContentSetting setting; On 2014/06/12 01:44:33, felt wrote: > why is this outside of the if-statement, but ti's only used within the scope of > the if-statement? Removed the if statement - it is not needed. https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:548: setting_is_wildcard) { On 2014/06/12 01:44:33, felt wrote: > ^ is that really over 80 chars? No it's not over 80 characters. Fixed it. https://codereview.chromium.org/319553008/diff/340001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:553: } else if ( setting == CONTENT_SETTING_ALLOW) { On 2014/06/12 01:44:33, felt wrote: > get rid of the unneeded space Done.
OK, time for you to add other reviewers. It looks like bauerb is OOO for the weekend, but in the meantime you can find reviewers for the other files: chrome/browser/ui/views/content_setting_bubble_contents.h chrome/browser/ui/views/content_setting_bubble_contents.cc chrome/renderer/chrome_content_renderer_client.cc https://codereview.chromium.org/319553008/diff/360001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/319553008/diff/360001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:478: // If the user elected to allow all plugins then run plugins at this time. If this comment is about the whole method, put it on the line above the method.
Updated changes. Should I email sky@ - he seems to be the common owner for all the three file? https://codereview.chromium.org/319553008/diff/360001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/319553008/diff/360001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:478: // If the user elected to allow all plugins then run plugins at this time. On 2014/06/12 17:18:48, felt wrote: > If this comment is about the whole method, put it on the line above the method. Done.
On 2014/06/12 17:44:21, radhikabhar wrote: > Updated changes. Should I email sky@ - he seems to be the common owner for all > the three file? > > https://codereview.chromium.org/319553008/diff/360001/chrome/browser/ui/conte... > File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): > > https://codereview.chromium.org/319553008/diff/360001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:478: // If > the user elected to allow all plugins then run plugins at this time. > On 2014/06/12 17:18:48, felt wrote: > > If this comment is about the whole method, put it on the line above the > method. > > Done. Sure, seems reasonable.
On 2014/06/12 17:46:13, felt wrote: > On 2014/06/12 17:44:21, radhikabhar wrote: > > Updated changes. Should I email sky@ - he seems to be the common owner for all > > the three file? > > > > > https://codereview.chromium.org/319553008/diff/360001/chrome/browser/ui/conte... > > File chrome/browser/ui/content_settings/content_setting_bubble_model.cc > (right): > > > > > https://codereview.chromium.org/319553008/diff/360001/chrome/browser/ui/conte... > > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:478: // If > > the user elected to allow all plugins then run plugins at this time. > > On 2014/06/12 17:18:48, felt wrote: > > > If this comment is about the whole method, put it on the line above the > > method. > > > > Done. > > Sure, seems reasonable. Hi Scott, Could you please review this code for me? This code updates the UI when a NPAPI plugin is blocked. Thanks!
I just want to make sure, was this approved by UX?
On 2014/06/12 19:43:08, sky wrote: > I just want to make sure, was this approved by UX? yes, I showed it to them
On 2014/06/12 19:47:42, felt wrote: > On 2014/06/12 19:43:08, sky wrote: > > I just want to make sure, was this approved by UX? > > yes, I showed it to them TODO(felt): ask ainslie@ to comment on the bug to make this self-evident
Can you get a local reviewer for the changes to chrome/browser/ui/content_settings ? https://codereview.chromium.org/319553008/diff/380001/chrome/browser/ui/brows... File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right): https://codereview.chromium.org/319553008/diff/380001/chrome/browser/ui/brows... chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:61: return; nit: no return. https://codereview.chromium.org/319553008/diff/380001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/319553008/diff/380001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:199: virtual void OnManageLinkClicked() OVERRIDE; nit: newline between 198 and 199 and prefix section with something like on line 108. https://codereview.chromium.org/319553008/diff/380001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/319553008/diff/380001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:805: observer->DidBlockContentType(content_type); Why isn't ChromeViewHostMsg_BlockedUnauthorizedPlugin enough?
bauerb@ should do the review for chrome/browser/ui/content_settings. can you please review: chrome/browser/ui/views/content_setting_bubble_contents.h chrome/browser/ui/views/content_setting_bubble_contents.cc chrome/renderer/chrome_content_renderer_client.cc On 2014/06/12 20:00:33, sky wrote: > Can you get a local reviewer for the changes to > chrome/browser/ui/content_settings ? > > https://codereview.chromium.org/319553008/diff/380001/chrome/browser/ui/brows... > File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right): > > https://codereview.chromium.org/319553008/diff/380001/chrome/browser/ui/brows... > chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:61: return; > nit: no return. > > https://codereview.chromium.org/319553008/diff/380001/chrome/browser/ui/conte... > File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): > > https://codereview.chromium.org/319553008/diff/380001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.h:199: virtual > void OnManageLinkClicked() OVERRIDE; > nit: newline between 198 and 199 and prefix section with something like on line > 108. > > https://codereview.chromium.org/319553008/diff/380001/chrome/renderer/chrome_... > File chrome/renderer/chrome_content_renderer_client.cc (right): > > https://codereview.chromium.org/319553008/diff/380001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client.cc:805: > observer->DidBlockContentType(content_type); > Why isn't ChromeViewHostMsg_BlockedUnauthorizedPlugin enough?
Updated changes https://codereview.chromium.org/319553008/diff/380001/chrome/browser/ui/brows... File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right): https://codereview.chromium.org/319553008/diff/380001/chrome/browser/ui/brows... chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:61: return; On 2014/06/12 20:00:33, sky wrote: > nit: no return. Done. https://codereview.chromium.org/319553008/diff/380001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/319553008/diff/380001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:199: virtual void OnManageLinkClicked() OVERRIDE; On 2014/06/12 20:00:33, sky wrote: > nit: newline between 198 and 199 and prefix section with something like on line > 108. Done. https://codereview.chromium.org/319553008/diff/380001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/319553008/diff/380001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:805: observer->DidBlockContentType(content_type); On 2014/06/12 20:00:33, sky wrote: > Why isn't ChromeViewHostMsg_BlockedUnauthorizedPlugin enough? In the new UI the infobar will be placed behind a finch trial and only the bubble model will be visible. It is linked to this change - https://codereview.chromium.org/300873002/
https://codereview.chromium.org/319553008/diff/380001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/319553008/diff/380001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:805: observer->DidBlockContentType(content_type); On 2014/06/12 20:37:19, radhikabhar wrote: > On 2014/06/12 20:00:33, sky wrote: > > Why isn't ChromeViewHostMsg_BlockedUnauthorizedPlugin enough? > > In the new UI the infobar will be placed behind a finch trial and only the > bubble model will be visible. It is linked to this change - > https://codereview.chromium.org/300873002/ I don't understand why that means a new message needs to be sent. Shouldn't the decision as to what UI to be shown not depend upon what message is sent? Said differently, why isn't ChromeViewHostMsg_BlockedUnauthorizedPlugin enough?
Question regarding the comment https://codereview.chromium.org/319553008/diff/380001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/319553008/diff/380001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:805: observer->DidBlockContentType(content_type); On 2014/06/12 22:56:31, sky wrote: > On 2014/06/12 20:37:19, radhikabhar wrote: > > On 2014/06/12 20:00:33, sky wrote: > > > Why isn't ChromeViewHostMsg_BlockedUnauthorizedPlugin enough? > > > > In the new UI the infobar will be placed behind a finch trial and only the > > bubble model will be visible. It is linked to this change - > > https://codereview.chromium.org/300873002/ > > I don't understand why that means a new message needs to be sent. Shouldn't the > decision as to what UI to be shown not depend upon what message is sent? Said > differently, why isn't ChromeViewHostMsg_BlockedUnauthorizedPlugin enough? One question - Are you thinking about adding this exception to change the status of the Unauthorized Plugins for NPAPI to Blocked above the switch statement in line 608 or why two IPC are being sent here? Also, I am actually not implementing the part of the plugin being displayed behind a Finch trial (that is being implemented in https://codereview.chromium.org/300873002) so should I just delete the IPC for showing the bubble model and revert it back to the original state and let the other CL handle this?
Never mind, I'm stupid, I was reading the code wrong. LGTM for everything but the chrome/browser/ui/content_settings changes.
bauerb@ Could you please review the code for the content_settings?
https://codereview.chromium.org/319553008/diff/400001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/319553008/diff/400001/chrome/app/generated_re... chrome/app/generated_resources.grd:2403: Block plug-ins on this site Did this string get UI-reviewed? It seems inconsistent with other strings used by content settings to change this (e.g. for Javascript blocking the string is "Continue blocking JavaScript" too). https://codereview.chromium.org/319553008/diff/400001/chrome/browser/content_... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/319553008/diff/400001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:261: // Set whether the setting for the plugin bubble mode is Allow, Block or Run Nit: "Model", not "mode". Also, can we please not use content settings for things that aren't content settings? What this property is is the selected *action*. One of the three values should change a content setting, one should run plugins, and one should do nothing at all. If you use content settings for this, you are going to mix up things. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:366: block_setting_ = setting; This isn't necessary anymore if this is not going to be used for plugins. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:460: int kAllowOnceButtonIndex; Why is this a member variable? https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:486: setting = CONTENT_SETTING_DEFAULT; Wait, this means that we will run all plugins currently on the page, but remove any setting for this site. Is this really the desired behavior? https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:488: } else { Can you DCHECK that the selected item is the block radio button? https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:493: if (profile()) { When is profile() NULL? https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:495: bubble_content().radio_group.url, Also, this will add an exception for a plugin with the URL of the current page, embedded on the current page (which doesn't make sense). Did you actually verify that this CL will have the correct effect on content settings? https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:502: bool ContentSettingPluginBubbleModel::settings_changed() const { This isn't really a trivial accessor method, so it shouldn't be named unix_hacker_style. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:514: bool allowed = I think this is only necessary for cookies. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:545: if (setting == CONTENT_SETTING_ALLOW && setting_is_wildcard) { Really, you could fold this case into the next one, and make the whole thing a switch statement. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:548: // whitelist. Are we actually still handling this case? We get |setting| from TabSpecificContentSettings, but there it is only set if the bubble closes, and the default value is BLOCK. So, for unrecognized plugins we will still get a BLOCK value out of TabSpecificContentSetting. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:549: radio_group.default_item = 2; Can this 2 also get a constant? https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:556: radio_group.default_item = 2; DCHECK that the setting is BLOCK? https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:562: set_radio_group_enabled(false); Extract `setting_source != SETTING_SOURCE_USER` into a local variable and use that for set_setting_is_managed() and set_radio_group_enabled()?
Also, is this CL really specific to NPAPI plugin blocking? It seems more like general, content-settings-based plugin blocking.
https://codereview.chromium.org/319553008/diff/400001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/319553008/diff/400001/chrome/app/generated_re... chrome/app/generated_resources.grd:2403: Block plug-ins on this site On 2014/06/16 09:43:15, Bernhard Bauer wrote: > Did this string get UI-reviewed? It seems inconsistent with other strings used > by content settings to change this (e.g. for Javascript blocking the string is > "Continue blocking JavaScript" too). Yes, but I'll ask Alex about whether we ought to change this.
Should there be two different UI for NPAPI and PPAPI plugin or should they be consistent for both the plugins? If it should be different then I can make the changes to revert the UI for the PPAPI plugins. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/content_... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/319553008/diff/400001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:261: // Set whether the setting for the plugin bubble mode is Allow, Block or Run On 2014/06/16 09:43:15, Bernhard Bauer wrote: > Nit: "Model", not "mode". Also, can we please not use content settings for > things that aren't content settings? What this property is is the selected > *action*. One of the three values should change a content setting, one should > run plugins, and one should do nothing at all. If you use content settings for > this, you are going to mix up things.\ Changed it and to remember the user selection for run this time added a boolean function https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:366: block_setting_ = setting; On 2014/06/16 09:43:16, Bernhard Bauer wrote: > This isn't necessary anymore if this is not going to be used for plugins. Removed it. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:460: int kAllowOnceButtonIndex; On 2014/06/16 09:43:15, Bernhard Bauer wrote: > Why is this a member variable? Removed it and made it as a global. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:486: setting = CONTENT_SETTING_DEFAULT; I added this for the corner case when the user selects Always run on this site and then Run this time. When the user selects "Always" it will add an exception with Allow but shouldn't this content setting be removed when he selects "Run this time". Is this the behavior that we want or should we remember that the user selected "Always run on this site" and let the content setting remain to allow? On 2014/06/16 09:43:16, Bernhard Bauer wrote: > Wait, this means that we will run all plugins currently on the page, but remove > any setting for this site. Is this really the desired behavior? https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:488: } else { On 2014/06/16 09:43:16, Bernhard Bauer wrote: > Can you DCHECK that the selected item is the block radio button? Done. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:493: if (profile()) { On 2014/06/16 09:43:15, Bernhard Bauer wrote: > When is profile() NULL? Removed it. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:495: bubble_content().radio_group.url, On 2014/06/16 09:43:15, Bernhard Bauer wrote: > Also, this will add an exception for a plugin with the URL of the current page, > embedded on the current page (which doesn't make sense). Did you actually verify > that this CL will have the correct effect on content settings? So previously the destructor was adding an exception for the URL and I just took it form there to do the same thing. I verified the effect on the content settings and it had the same effect with and without the patch. Is there something I am missing or anything else that I should be doing? https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:502: bool ContentSettingPluginBubbleModel::settings_changed() const { On 2014/06/16 09:43:15, Bernhard Bauer wrote: > This isn't really a trivial accessor method, so it shouldn't be named > unix_hacker_style. Done. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:514: bool allowed = On 2014/06/16 09:43:15, Bernhard Bauer wrote: > I think this is only necessary for cookies. Done. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:545: if (setting == CONTENT_SETTING_ALLOW && setting_is_wildcard) { On 2014/06/16 09:43:16, Bernhard Bauer wrote: > Really, you could fold this case into the next one, and make the whole thing a > switch statement. Since I changed the way I remember the user selection for run this time I still do it as an if statement. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:548: // whitelist. On 2014/06/16 09:43:16, Bernhard Bauer wrote: > Are we actually still handling this case? We get |setting| from > TabSpecificContentSettings, but there it is only set if the bubble closes, and > the default value is BLOCK. So, for unrecognized plugins we will still get a > BLOCK value out of TabSpecificContentSetting. Removed it https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:549: radio_group.default_item = 2; On 2014/06/16 09:43:16, Bernhard Bauer wrote: > Can this 2 also get a constant? Done. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:556: radio_group.default_item = 2; On 2014/06/16 09:43:16, Bernhard Bauer wrote: > DCHECK that the setting is BLOCK? Done. https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:562: set_radio_group_enabled(false); On 2014/06/16 09:43:15, Bernhard Bauer wrote: > Extract `setting_source != SETTING_SOURCE_USER` into a local variable and use > that for set_setting_is_managed() and set_radio_group_enabled()? Done.
On 2014/06/16 18:21:38, radhikabhar wrote: > Should there be two different UI for NPAPI and PPAPI plugin or should they be > consistent for both the plugins? If it should be different then I can make the > changes to revert the UI for the PPAPI plugins. > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/content_... > File chrome/browser/content_settings/tab_specific_content_settings.h (right): > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/content_... > chrome/browser/content_settings/tab_specific_content_settings.h:261: // Set > whether the setting for the plugin bubble mode is Allow, Block or Run > On 2014/06/16 09:43:15, Bernhard Bauer wrote: > > Nit: "Model", not "mode". Also, can we please not use content settings for > > things that aren't content settings? What this property is is the selected > > *action*. One of the three values should change a content setting, one should > > run plugins, and one should do nothing at all. If you use content settings for > > this, you are going to mix up things.\ > > Changed it and to remember the user selection for run this time added a boolean > function > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:366: > block_setting_ = setting; > On 2014/06/16 09:43:16, Bernhard Bauer wrote: > > This isn't necessary anymore if this is not going to be used for plugins. > > Removed it. > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:460: int > kAllowOnceButtonIndex; > On 2014/06/16 09:43:15, Bernhard Bauer wrote: > > Why is this a member variable? > > Removed it and made it as a global. > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:486: setting > = CONTENT_SETTING_DEFAULT; > I added this for the corner case when the user selects Always run on this site > and then Run this time. When the user selects "Always" it will add an exception > with Allow but shouldn't this content setting be removed when he selects "Run > this time". Is this the behavior that we want or should we remember that the > user selected "Always run on this site" and let the content setting remain to > allow? > On 2014/06/16 09:43:16, Bernhard Bauer wrote: > > Wait, this means that we will run all plugins currently on the page, but > remove > > any setting for this site. Is this really the desired behavior? > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:488: } else { > On 2014/06/16 09:43:16, Bernhard Bauer wrote: > > Can you DCHECK that the selected item is the block radio button? > > Done. > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:493: if > (profile()) { > On 2014/06/16 09:43:15, Bernhard Bauer wrote: > > When is profile() NULL? > > Removed it. > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:495: > bubble_content().radio_group.url, > On 2014/06/16 09:43:15, Bernhard Bauer wrote: > > Also, this will add an exception for a plugin with the URL of the current > page, > > embedded on the current page (which doesn't make sense). Did you actually > verify > > that this CL will have the correct effect on content settings? > > So previously the destructor was adding an exception for the URL and I just took > it form there to do the same thing. I verified the effect on the content > settings and it had the same effect with and without the patch. > > Is there something I am missing or anything else that I should be doing? > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:502: bool > ContentSettingPluginBubbleModel::settings_changed() const { > On 2014/06/16 09:43:15, Bernhard Bauer wrote: > > This isn't really a trivial accessor method, so it shouldn't be named > > unix_hacker_style. > > Done. > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:514: bool > allowed = > On 2014/06/16 09:43:15, Bernhard Bauer wrote: > > I think this is only necessary for cookies. > > Done. > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:545: if > (setting == CONTENT_SETTING_ALLOW && setting_is_wildcard) { > On 2014/06/16 09:43:16, Bernhard Bauer wrote: > > Really, you could fold this case into the next one, and make the whole thing a > > switch statement. > > Since I changed the way I remember the user selection for run this time I still > do it as an if statement. > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:548: // > whitelist. > On 2014/06/16 09:43:16, Bernhard Bauer wrote: > > Are we actually still handling this case? We get |setting| from > > TabSpecificContentSettings, but there it is only set if the bubble closes, and > > the default value is BLOCK. So, for unrecognized plugins we will still get a > > BLOCK value out of TabSpecificContentSetting. > > Removed it > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:549: > radio_group.default_item = 2; > On 2014/06/16 09:43:16, Bernhard Bauer wrote: > > Can this 2 also get a constant? > > Done. > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:556: > radio_group.default_item = 2; > On 2014/06/16 09:43:16, Bernhard Bauer wrote: > > DCHECK that the setting is BLOCK? > > Done. > > https://codereview.chromium.org/319553008/diff/400001/chrome/browser/ui/conte... > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:562: > set_radio_group_enabled(false); > On 2014/06/16 09:43:15, Bernhard Bauer wrote: > > Extract `setting_source != SETTING_SOURCE_USER` into a local variable and use > > that for set_setting_is_managed() and set_radio_group_enabled()? > > Done. Updated the CL so that the only difference between the new UI and the old UI is the "Learn More" link.
This change LGTM, but it might be easier to just create a new CL if the scope changes that much. Also, the description is now really out of date. (What I initially refered to with the comment about NPAPI was that this UI is not specific to NPAPI, but the CL description says it is.)
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/319553008/480001
On 2014/06/17 17:27:41, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/319553008/480001 Radhika, you need to update the description like Bernhard asked.
On 2014/06/17 17:29:56, felt wrote: > On 2014/06/17 17:27:41, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/319553008/480001 > > Radhika, you need to update the description like Bernhard asked. Updated the description.
On 2014/06/17 17:34:38, radhikabhar wrote: > On 2014/06/17 17:29:56, felt wrote: > > On 2014/06/17 17:27:41, I haz the power (commit-bot) wrote: > > > CQ is trying da patch. Follow status at > > > > > > > > > https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/319553008/480001 > > > > Radhika, you need to update the description like Bernhard asked. > > Updated the description. Thanks. Please update the title as well (in particular, the title should match the first line of the description, because only the description is going to become the commit message).
On 2014/06/17 18:45:48, Bernhard Bauer wrote: > On 2014/06/17 17:34:38, radhikabhar wrote: > > On 2014/06/17 17:29:56, felt wrote: > > > On 2014/06/17 17:27:41, I haz the power (commit-bot) wrote: > > > > CQ is trying da patch. Follow status at > > > > > > > > > > > > > > https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/319553008/480001 > > > > > > Radhika, you need to update the description like Bernhard asked. > > > > Updated the description. > > Thanks. Please update the title as well (in particular, the title should match > the first line of the description, because only the description is going to > become the commit message). Updated the title
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
On 2014/06/17 18:45:48, Bernhard Bauer wrote: > (in particular, the title should match > the first line of the description, because only the description is going to > become the commit message). ^^^
On 2014/06/18 08:38:10, Bernhard Bauer wrote: > On 2014/06/17 18:45:48, Bernhard Bauer wrote: > > (in particular, the title should match > > the first line of the description, because only the description is going to > > become the commit message). > > ^^^ Updated the title and the description.
LGTM. I approve of the term "sliding yellow thing" :)
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/319553008/500001
Message was sent while issue was closed.
Change committed as 278302 |
