Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1576183003:
chrome://plugins Mojo-ification part 2/2, populating the page. (Closed)
|
Created:
4 years, 11 months ago by dpapad Modified:
4 years, 10 months ago CC:
Aaron Boodman, abarth-chromium, arv+watch_chromium.org, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@plugins_mojo1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionchrome://plugins Mojo-ification part 2/2, populating the page.
BUG=570143
Committed: https://crrev.com/105cb5f2d9d1a3f2045cef5b7efd3bbfbda69e90
Cr-Commit-Position: refs/heads/master@{#372268}
Patch Set 1 #Patch Set 2 : Cleanups + always_allowed #Patch Set 3 : Observer method fires, need to call JS #Patch Set 4 : More work #
Total comments: 11
Patch Set 5 : rebasing #Patch Set 6 : Merge part2 and part3 #Patch Set 7 : JS cleanups #Patch Set 8 : DOMContentLoaded #Patch Set 9 : Update documentation #Patch Set 10 : Rebasing, removing call to AddMojoResources() #
Total comments: 27
Patch Set 11 : Addressing comments. #
Total comments: 16
Patch Set 12 : Addressing comments. #Patch Set 13 : Address comments. #
Total comments: 1
Patch Set 14 : Nit #
Messages
Total messages: 34 (16 generated)
Description was changed from ========== chrome://plugins Mojo-ification, rebased, WIP. BUG= ========== to ========== chrome://plugins Mojo-ification, rebased, WIP. BUG=570143 ==========
Patchset #4 (id:60001) has been deleted
Description was changed from ========== chrome://plugins Mojo-ification, rebased, WIP. BUG=570143 ========== to ========== chrome://plugins Mojo-ification part 2/3, populating the page. BUG=570143 ==========
sky@chromium.org changed reviewers: + sky@chromium.org
I also encourage you to file bugs for areas that are not particularly straightforward. For example, the subtlety of null with arrays is a bit confusing and easy to get wrong. https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/plugins/plugins.mojom (right): https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/plugins/plugins.mojom:28: array<PluginFile> plugin_files; Arrays and other non-POD types support null. When you declare an array like it means the array is never null (an error is generated by bindings code if you attempt to send a null). If you create an array like this: mojo::Array<MimeTypePtr> mime_types; it's null. Instead if you do: mojo::Array<MimeTypePtr> mime_types(0u); it isn't null. Adding an element to the array implicitly makes it non-null. If you really want to allow for a null array than you use ?, eg: array<PluginFile>? plugin_files; I mention this as a handful of places in your patch you might end up creating an array as null. https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/plugins/plugins_handler.cc (right): https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/plugins/plugins_handler.cc:93: mojo::Array<MimeTypePtr> mime_types; If plugin.mime_types is empty, then mime_types is null, and generates an error. If you supply the size to the constructor than it won't be null (0 works too). Supplying the size means you need to change from a push_back to []. https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/plugins/plugins_handler.cc:105: mime_type->fileExtensions = std::move(file_extensions); You can nuke 100-103 and use: mime_type->fileExtensions = mojo::Array<mojo::String>::From( plugin_mime_type.file_extensions); https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/plugins/plugins_handler.cc:137: enable, base::FilePath(plugin_path.To<base::StringPiece>()), I think you want: base::FilePath(plugin_path.To<base::FilePath::StringType>()), here. https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/plugins/plugins_handler.cc:232: mojo::Array<PluginDataPtr> plugins_data; This is another place where you end up with a null array if groups is empty. https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/plugins/plugins_handler.cc:247: mojo::Array<PluginFilePtr> plugin_files; And this one too.
Description was changed from ========== chrome://plugins Mojo-ification part 2/3, populating the page. BUG=570143 ========== to ========== chrome://plugins Mojo-ification part 2/2, populating the page. BUG=570143 ==========
Patchset #10 (id:200001) has been deleted
Patchset #10 (id:220001) has been deleted
Patchset #10 (id:240001) has been deleted
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/plugins/plugins_handler.cc (right): https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/plugins/plugins_handler.cc:93: mojo::Array<MimeTypePtr> mime_types; On 2016/01/20 at 19:23:08, sky wrote: > If plugin.mime_types is empty, then mime_types is null, and generates an error. If you supply the size to the constructor than it won't be null (0 works too). Supplying the size means you need to change from a push_back to []. Done. I supplied "0u" as the size and kept using push_back, since I am using "for" in a way that does not give me the iteration index, and prefer not to keep track of it manually (here and elsewhere in this file where a mojo::Array is declared). Also, thanks for filing crbug.com/579634 for this issue. https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/plugins/plugins_handler.cc:105: mime_type->fileExtensions = std::move(file_extensions); On 2016/01/20 at 19:23:08, sky wrote: > You can nuke 100-103 and use: > mime_type->fileExtensions = mojo::Array<mojo::String>::From( > plugin_mime_type.file_extensions); Done! https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/plugins/plugins_handler.cc:137: enable, base::FilePath(plugin_path.To<base::StringPiece>()), On 2016/01/20 at 19:23:08, sky wrote: > I think you want: > base::FilePath(plugin_path.To<base::FilePath::StringType>()), > here. Done. https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/plugins/plugins_handler.cc:232: mojo::Array<PluginDataPtr> plugins_data; On 2016/01/20 at 19:23:08, sky wrote: > This is another place where you end up with a null array if groups is empty. Fixed. https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/plugins/plugins_handler.cc:247: mojo::Array<PluginFilePtr> plugin_files; On 2016/01/20 at 19:23:08, sky wrote: > And this one too. Fixed too.
https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/plugins.html (right): https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.html:74: <span jsdisplay="enabled_mode == 'disabledByUser'" can you do these changes separately? https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/plugins.js (right): https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.js:59: * file_extensions: [ 'pdf'], [\s -> [ https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.js:64: * file_extensions: [ 'bar', 'baz'], [\s -> [ https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.js:79: * file_extensions: [ 'mfp'], [\s -> [ https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.js:269: var whenBrowserProxyReady = getBrowserProxy_(); creating a method just to call it once here adds no benefit https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.js:281: function getBrowserProxy_() { only @private methods or members should have a trailing _ https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.js:321: function whenDomContentLoaded() { this doesn't need to be a function https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/plugins/plugins_handler.cc (right): https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:93: mojo::Array<MimeTypePtr> mime_types(0u); shouldn't this be initialized with plugin.mime_types.size()? https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:114: weak_ptr_factory_(this) { i think the style-guide (or previous reviewers of mine?) have preferred multi-line initializers be 1-per-line, tl;dr - web_ui_(web_ui), binding_(this, std::move(request)), weak_ptr_factory_(this) { but i honestly don't remember that this is a hard rule https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:181: if (weak_ptr_factory_.HasWeakPtrs()) what is this supposed to be doing? seeing if there are any other things in flight? https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:233: mojo::Array<PluginDataPtr> plugins_data(0u); init with groups.size()? https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:248: mojo::Array<PluginFilePtr> plugin_files(0u); shouldn't this be initialized with group_plugins.size()? https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/plugins/plugins_handler.h (right): https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.h:22: public content::NotificationObserver { wrong indent https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.h:81: content::WebUI* web_ui_; nit: document ownership
https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/plugins.html (right): https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.html:74: <span jsdisplay="enabled_mode == 'disabledByUser'" On 2016/01/26 at 20:10:43, Dan Beam wrote: > can you do these changes separately? This is tied to the mojom interface at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... Doing it separately would require changing the interface (and C+ handler) to use camel-case first, then land this CL, then change the interface and handler again to use underscores again. Does not seem worth it. https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/plugins.js (right): https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.js:59: * file_extensions: [ 'pdf'], On 2016/01/26 at 20:10:43, Dan Beam wrote: > [\s -> [ Changed. https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.js:64: * file_extensions: [ 'bar', 'baz'], On 2016/01/26 at 20:10:43, Dan Beam wrote: > [\s -> [ Changed. https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.js:79: * file_extensions: [ 'mfp'], On 2016/01/26 at 20:10:43, Dan Beam wrote: > [\s -> [ Changed. https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.js:269: var whenBrowserProxyReady = getBrowserProxy_(); On 2016/01/26 at 20:10:43, Dan Beam wrote: > creating a method just to call it once here adds no benefit Changed. https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.js:281: function getBrowserProxy_() { On 2016/01/26 at 20:10:43, Dan Beam wrote: > only @private methods or members should have a trailing _ Removed this function. https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/resourc... chrome/browser/resources/plugins.js:321: function whenDomContentLoaded() { On 2016/01/26 at 20:10:43, Dan Beam wrote: > this doesn't need to be a function Changed. https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/plugins/plugins_handler.cc (right): https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:93: mojo::Array<MimeTypePtr> mime_types(0u); On 2016/01/26 at 20:10:43, Dan Beam wrote: > shouldn't this be initialized with plugin.mime_types.size()? Does not have to. There are two ways to populate a mojo::Array 1) Initialize size to 0u and use push_back(), which grows the array as necessary. 2) Initialize size to a non-zero value and use my_array[index] = 'foo' (instead of push_back). I chose approach number 1, because it allows me to not manually keep track of 'index' within the modern-style 'for' loop. https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:114: weak_ptr_factory_(this) { On 2016/01/26 at 20:10:43, Dan Beam wrote: > i think the style-guide (or previous reviewers of mine?) have preferred multi-line initializers be 1-per-line, tl;dr - > > web_ui_(web_ui), > binding_(this, std::move(request)), > weak_ptr_factory_(this) { > > but i honestly don't remember that this is a hard rule Changed (along with other style fixes, just run clang-format on this file). https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:233: mojo::Array<PluginDataPtr> plugins_data(0u); On 2016/01/26 at 20:10:43, Dan Beam wrote: > init with groups.size()? See previous response to similar question. https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:248: mojo::Array<PluginFilePtr> plugin_files(0u); On 2016/01/26 at 20:10:43, Dan Beam wrote: > shouldn't this be initialized with group_plugins.size()? See previous response to similar question. https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/plugins/plugins_handler.h (right): https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.h:22: public content::NotificationObserver { On 2016/01/26 at 20:10:43, Dan Beam wrote: > wrong indent Fixed (along with other style issues, just run clang-format on this file). https://codereview.chromium.org/1576183003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.h:81: content::WebUI* web_ui_; On 2016/01/26 at 20:10:43, Dan Beam wrote: > nit: document ownership Done.
Friendly ping.
lgtm
I kind of glossed over the creation of mojo objects in the handlers: I assume
sky@ knows more about that than me. very cool that we get auto-generated,
strongly-typed structs instead of base::Dictionary::SetThing("easyToMistype").
I still think it's better to allocate the mojo arrays to an explicit size when
we know how big they'll be, but it probably makes no tangible difference in
performance/memory allocation churn in this case.
https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/resourc...
File chrome/browser/resources/plugins.js (right):
https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/resourc...
chrome/browser/resources/plugins.js:23: tmiModeExpanded = showDetails;
arguably: move the declaration of tmiModeExpanded to before it's first use, i.e.
var tmiModeExpanded = false;
function loadShowDetailsFromPrefs(showDetails) {
tmiModeExpanded = showDetails;
...
}
https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/resourc...
chrome/browser/resources/plugins.js:182: isGroup ?
don't do this
https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/resourc...
chrome/browser/resources/plugins.js:301:
bindings.StubBindings(pluginsPageStub).delegate = new PluginsPageImpl();
nit: this seems to be simpler (and mildly akin to Java's anonymous classes, if
that's a good thing...)
bindings.StubBindings(pluginsPageStub).delegate = {
__proto__: pluginsMojom.PluginsPageMojo.stubClass.prototype,
onPluginsUpdated: function() {
returnPluginsData({plugins: plugins});
},
};
are we compiling this code yet?
https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/plugins/plugins_handler.cc (right):
https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/plugins/plugins_handler.cc:140: if (enable) {
optional nit:
if (!enable)
return;
... less indented code ...
https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/plugins/plugins_handler.cc (right): https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:209: page_->OnPluginsUpdated(GeneratePluginsData(plugins)); As page_ is not immediately set you need an if (page_) here https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/plugins_ui.h (right): https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins_ui.h:21: class PluginsUI : public MojoWebUIController<PluginsHandlerMojo> { I assume this is from earlier work and you don't intend to actually commit this, right?
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1576183003/#ps300001 (title: "Addressing comments.")
The CQ bit was unchecked by dpapad@chromium.org
Patchset #12 (id:300001) has been deleted
https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/resourc... File chrome/browser/resources/plugins.js (right): https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/resourc... chrome/browser/resources/plugins.js:23: tmiModeExpanded = showDetails; On 2016/01/28 at 04:25:08, Dan Beam wrote: > arguably: move the declaration of tmiModeExpanded to before it's first use, i.e. > > var tmiModeExpanded = false; > > function loadShowDetailsFromPrefs(showDetails) { > tmiModeExpanded = showDetails; > ... > } Done. https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/resourc... chrome/browser/resources/plugins.js:182: isGroup ? On 2016/01/28 at 04:25:08, Dan Beam wrote: > don't do this It's not apparent why you are suggesting not to do this. Can you explain? I have been using this pattern a lot (in other codebases) and find it very convenient. https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/resourc... chrome/browser/resources/plugins.js:301: bindings.StubBindings(pluginsPageStub).delegate = new PluginsPageImpl(); On 2016/01/28 at 04:25:08, Dan Beam wrote: > nit: this seems to be simpler (and mildly akin to Java's anonymous classes, if that's a good thing...) > > bindings.StubBindings(pluginsPageStub).delegate = { > __proto__: pluginsMojom.PluginsPageMojo.stubClass.prototype, > onPluginsUpdated: function() { > returnPluginsData({plugins: plugins}); > }, > }; > > are we compiling this code yet? Done. This is not compiled yet. See https://code.google.com/p/chromium/issues/detail?id=574512#c3 for more context. Wanted to chat about it with you anyway (independently of this CL though). https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/plugins/plugins_handler.cc (right): https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:140: if (enable) { On 2016/01/28 at 04:25:08, Dan Beam wrote: > optional nit: > > if (!enable) > return; > > ... less indented code ... Done. https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:209: page_->OnPluginsUpdated(GeneratePluginsData(plugins)); On 2016/01/28 at 16:45:27, sky wrote: > As page_ is not immediately set you need an if (page_) here Done. https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/plugins_ui.h (right): https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins_ui.h:21: class PluginsUI : public MojoWebUIController<PluginsHandlerMojo> { On 2016/01/28 at 16:45:27, sky wrote: > I assume this is from earlier work and you don't intend to actually commit this, right? I was following the example from site_engagement and omnibox, see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... and https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... Isn't it necessary?
https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/resourc... File chrome/browser/resources/plugins.js (right): https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/resourc... chrome/browser/resources/plugins.js:182: isGroup ? On 2016/01/28 18:32:54, dpapad wrote: > On 2016/01/28 at 04:25:08, Dan Beam wrote: > > don't do this > > It's not apparent why you are suggesting not to do this. Can you explain? if (isGroup) browserProxy.setPluginGroupEnabled(node.path, enable); else browserProxy.setPluginEnabled(node.path, enable) > I have > been using this pattern a lot (in other codebases) and find it very convenient. feel free to do it in other codebases as much as you'd like (or find precedent for this in chrome) https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/plugins_ui.h (right): https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins_ui.h:21: class PluginsUI : public MojoWebUIController<PluginsHandlerMojo> { On 2016/01/28 18:32:54, dpapad wrote: > On 2016/01/28 at 16:45:27, sky wrote: > > I assume this is from earlier work and you don't intend to actually commit > this, right? > > I was following the example from site_engagement and omnibox, see > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > and > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... > Isn't it necessary? those actually have .cc files to implement them this just looks like a botched file move (note the path doesn't have "/plugins/" in it like "chrome/browser/ui/webui/plugins/plugins_ui.h" does)
https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/resourc... File chrome/browser/resources/plugins.js (right): https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/resourc... chrome/browser/resources/plugins.js:182: isGroup ? On 2016/01/28 at 19:20:34, Dan Beam wrote: > On 2016/01/28 18:32:54, dpapad wrote: > > On 2016/01/28 at 04:25:08, Dan Beam wrote: > > > don't do this > > > > It's not apparent why you are suggesting not to do this. Can you explain? > > if (isGroup) > browserProxy.setPluginGroupEnabled(node.path, enable); > else > browserProxy.setPluginEnabled(node.path, enable) > > > I have > > been using this pattern a lot (in other codebases) and find it very convenient. > > feel free to do it in other codebases as much as you'd like (or find precedent for this in chrome) Changed per suggestion. Finding precedent for something is not golden criterion though. By induction, if whenever something with no precedent is turned down, there can never be precedent. The question of whether to allow or disallow something should not be based just on precedence but on quite a few factors (is it useful? requires less visual clutter? is it clear to the reader?). The styleguide serves better as a mean to restrict language usage to a subset, so I suggest making a case there if you prefer not to repeat the same conversation with another code reviewee in the future. https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/plugins_ui.h (right): https://codereview.chromium.org/1576183003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins_ui.h:21: class PluginsUI : public MojoWebUIController<PluginsHandlerMojo> { On 2016/01/28 at 19:20:34, Dan Beam wrote: > On 2016/01/28 18:32:54, dpapad wrote: > > On 2016/01/28 at 16:45:27, sky wrote: > > > I assume this is from earlier work and you don't intend to actually commit > > this, right? > > > > I was following the example from site_engagement and omnibox, see > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > and > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... > > Isn't it necessary? > > those actually have .cc files to implement them > > this just looks like a botched file move (note the path doesn't have "/plugins/" in it like "chrome/browser/ui/webui/plugins/plugins_ui.h" does) Ah, I see now. Yes, this was a stale file in this CL from some previous intermediate step. Removed.
lgtm
LGTM https://codereview.chromium.org/1576183003/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/plugins/plugins_handler.cc (right): https://codereview.chromium.org/1576183003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/plugins/plugins_handler.cc:211: if (page_) { nit: no {}
Thanks! @sky: I recall you mentioning (over email) that some Windows specific changes might be needed, but did not see any in the review, are there any? I also have not done manual testing on Windows, but will do this (hopefully I can borrow somebody's machine), before landing.
On 2016/01/28 22:12:42, dpapad wrote: > Thanks! > > @sky: I recall you mentioning (over email) that some Windows specific changes > might be needed, but did not see any in the review, are there any? I also have > not done manual testing on Windows, but will do this (hopefully I can borrow > somebody's machine), before landing. lgtm on windows (just built there; TMI, disabling, enabling, disabling specific plugins, and the UI all worked fine)
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1576183003/#ps360001 (title: "Nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1576183003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1576183003/360001
Message was sent while issue was closed.
Committed patchset #14 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== chrome://plugins Mojo-ification part 2/2, populating the page. BUG=570143 ========== to ========== chrome://plugins Mojo-ification part 2/2, populating the page. BUG=570143 Committed: https://crrev.com/105cb5f2d9d1a3f2045cef5b7efd3bbfbda69e90 Cr-Commit-Position: refs/heads/master@{#372268} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/105cb5f2d9d1a3f2045cef5b7efd3bbfbda69e90 Cr-Commit-Position: refs/heads/master@{#372268}
Message was sent while issue was closed.
On Thu, Jan 28, 2016 at 2:12 PM, dpapad@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Thanks! > > @sky: I recall you mentioning (over email) that some Windows specific > changes > might be needed, but did not see any in the review, are there any? It was a compile issue, which you must have fixed otherwise it wouldn't land. I believe it was this one: https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/ui/webui... -Scott > I also > have > not done manual testing on Windows, but will do this (hopefully I can borrow > somebody's machine), before landing. > > https://codereview.chromium.org/1576183003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
