Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2862031:
Add click-to-load functionality for blocked plugins. (Closed)
|
Created:
10 years, 5 months ago by Bernhard Bauer Modified:
9 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, jam Base URL:
git://codf21.jail/chromium.git Visibility:
Public. |
DescriptionAdd click-to-load functionality for blocked plugins.
BUG=35316
TEST=Disable plugins, go to a site with a flash animation. You should see a placeholder for the flash animation. When you click on it, the animation should load.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52899
Patch Set 1 #Patch Set 2 : Remove unnecessary file. #Patch Set 3 : Fix compile error. #Patch Set 4 : Remove plugin.png for testing. #Patch Set 5 : Fix Skia compile error. #Patch Set 6 : fix win compile error? #Patch Set 7 : '' #Patch Set 8 : fix a typo #Patch Set 9 : Move blocked plugin initialization into BlockedPlugin #
Total comments: 2
Patch Set 10 : fix a crash #Patch Set 11 : add virtual destructor to domuiplugindelegate #
Total comments: 21
Patch Set 12 : feedback #Patch Set 13 : '' #
Total comments: 3
Patch Set 14 : '' #
Messages
Total messages: 22 (0 generated)
Could you please review this?
Drive-by bikeshed http://codereview.chromium.org/2862031/diff/21001/22001 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/2862031/diff/21001/22001#newcode3704 chrome/app/generated_resources.grd:3704: Click here to load plugin Just "Load plug-in"? (We use "Plug-in" instead of "Plugin" everywhere)
One thing occurred to me about the design of this. We have previously tried to avoid loading chrome:// URLs into the same process that handles http:// URLs. We do that to limit the damage that can be caused by a corrupt renderer. A corrupt renderer that can load chrome:// URLs might be able to do harmful things. Normally, when designing chrome:// pages, we like to avoid having to worry about them being executed by a possibly corrupt renderer. This is all to say that you might need to implement the blocked page UI by using something other than chrome:// URLs. Maybe you should just a data: URL to load a snot of HTML.
On 2010/07/08 06:34:39, darin wrote: > One thing occurred to me about the design of this. We have previously tried to > avoid loading chrome:// URLs into the same process that handles http:// URLs. > We do that to limit the damage that can be caused by a corrupt renderer. A > corrupt renderer that can load chrome:// URLs might be able to do harmful > things. Normally, when designing chrome:// pages, we like to avoid having to > worry about them being executed by a possibly corrupt renderer. > > This is all to say that you might need to implement the blocked page UI by using > something other than chrome:// URLs. Maybe you should just a data: URL to load > a snot of HTML. Or just use WebFrame::loadHTMLString to load the HTML content into your embedded WebView.
http://codereview.chromium.org/2862031/diff/21001/22001 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/2862031/diff/21001/22001#newcode3704 chrome/app/generated_resources.grd:3704: Click here to load plugin On 2010/07/07 00:02:32, Nico wrote: > Just "Load plug-in"? > > (We use "Plug-in" instead of "Plugin" everywhere) OK. Are there wording guidelines for Chrome somewhere?
On 2010/07/08 06:35:08, darin wrote: > On 2010/07/08 06:34:39, darin wrote: > > One thing occurred to me about the design of this. We have previously tried > to > > avoid loading chrome:// URLs into the same process that handles http:// URLs. > > We do that to limit the damage that can be caused by a corrupt renderer. A > > corrupt renderer that can load chrome:// URLs might be able to do harmful > > things. Normally, when designing chrome:// pages, we like to avoid having to > > worry about them being executed by a possibly corrupt renderer. > > > > This is all to say that you might need to implement the blocked page UI by > using > > something other than chrome:// URLs. Maybe you should just a data: URL to > load > > a snot of HTML. > > Or just use WebFrame::loadHTMLString to load the HTML content into your embedded > WebView. That's exactly what I'm doing, but I need to pass a base URL to loadHTMLString, which is never directly loaded, but passed to the FrameLoader and later on to WebFrameClient::canHandleRequest (which DOMUIPlugin implements). I picked a chrome:// URL so that I can choose not to load HTTP URLs there and don't even get near making a network request.
On 2010/07/08 09:23:32, Bernhard Bauer wrote: > On 2010/07/08 06:35:08, darin wrote: > > On 2010/07/08 06:34:39, darin wrote: > > > One thing occurred to me about the design of this. We have previously tried > > to > > > avoid loading chrome:// URLs into the same process that handles http:// > URLs. > > > We do that to limit the damage that can be caused by a corrupt renderer. A > > > corrupt renderer that can load chrome:// URLs might be able to do harmful > > > things. Normally, when designing chrome:// pages, we like to avoid having > to > > > worry about them being executed by a possibly corrupt renderer. > > > > > > This is all to say that you might need to implement the blocked page UI by > > using > > > something other than chrome:// URLs. Maybe you should just a data: URL to > > load > > > a snot of HTML. > > > > Or just use WebFrame::loadHTMLString to load the HTML content into your > embedded > > WebView. > > That's exactly what I'm doing, but I need to pass a base URL to loadHTMLString, > which is never directly loaded, but passed to the FrameLoader and later on to > WebFrameClient::canHandleRequest (which DOMUIPlugin implements). I picked a > chrome:// URL so that I can choose not to load HTTP URLs there and don't even > get near making a network request. That makes sense. I must have misread the code.
Ping?
really sorry... this fell off my radar :( http://codereview.chromium.org/2862031/diff/31001/32003 File chrome/renderer/blocked_plugin.cc (right): http://codereview.chromium.org/2862031/diff/31001/32003#newcode48 chrome/renderer/blocked_plugin.cc:48: NOTREACHED() << "unable to load template. ID: " << resource_id; can this just be a DCHECK? as is, if this were to fail in a release build, you would just have a silent return. maybe a CHECK could be used if you think there is a possibility of this failing in a production build. the early return just seems like a good way to have a mysterious failure in a production build, and doesn't add anything to a debug build. http://codereview.chromium.org/2862031/diff/31001/32003#newcode62 chrome/renderer/blocked_plugin.cc:62: WebURL(), false); nit: you can just leave off the optional 3rd and 4th parameters. http://codereview.chromium.org/2862031/diff/31001/32003#newcode75 chrome/renderer/blocked_plugin.cc:75: CHECK(plugin_); can this be a DCHECK? if plugin_ is null, i think you'll get a crash report on the next line, dereferencing container. http://codereview.chromium.org/2862031/diff/31001/32003#newcode77 chrome/renderer/blocked_plugin.cc:77: WebPlugin* newPlugin = nit: newPlugin -> new_plugin http://codereview.chromium.org/2862031/diff/31001/32005 File chrome/renderer/dom_ui_plugin.cc (right): http://codereview.chromium.org/2862031/diff/31001/32005#newcode62 chrome/renderer/dom_ui_plugin.cc:62: gfx::Rect paintRect(rect_.Intersect(rect)); nit: paintRect -> paint_rect http://codereview.chromium.org/2862031/diff/31001/32005#newcode66 chrome/renderer/dom_ui_plugin.cc:66: paintRect.Offset(-rect_.x(), -rect_.y()); does all of this transformation stuff work properly when the plugin is embedded within an iframe? what about if the containing page is scrolled and/or the iframe is scrolled? http://codereview.chromium.org/2862031/diff/31001/32005#newcode73 chrome/renderer/dom_ui_plugin.cc:73: skia::PlatformCanvas* platformCanvas = canvas; nit: platformCanvas -> platform_canvas http://codereview.chromium.org/2862031/diff/31001/32005#newcode78 chrome/renderer/dom_ui_plugin.cc:78: if (needs_layout_) { the WebView may require layout for internal reasons. you have to call layout before paint. webkit has a dirty bit system, so layout will be a no-op if there is no work to be done. http://codereview.chromium.org/2862031/diff/31001/32005#newcode108 chrome/renderer/dom_ui_plugin.cc:108: cursor_info_ = &cursor; hmm... what if didChangeCursor happens outside the context of handleInputEvent? i'd recommend just having a WebCursorInfo member, call it current_cursor_, and then at the end of this function set cursor = current_cursor_ before returning. then in didChangeCursor just set current_cursor_. http://codereview.chromium.org/2862031/diff/31001/32005#newcode146 chrome/renderer/dom_ui_plugin.cc:146: return url.SchemeIs("chrome"); nit: return GURL(request.url()).SchemeIs("chrome"); http://codereview.chromium.org/2862031/diff/31001/32005#newcode153 chrome/renderer/dom_ui_plugin.cc:153: WebURLError error; you should also initialize the domain member, perhaps to a value like "DOMUIPlugin" (or "WebViewPlugin" if the class is renamed). http://codereview.chromium.org/2862031/diff/31001/32006 File chrome/renderer/dom_ui_plugin.h (right): http://codereview.chromium.org/2862031/diff/31001/32006#newcode15 chrome/renderer/dom_ui_plugin.h:15: virtual void BindWebFrame(WebKit::WebFrame* frame) = 0; nit: please add some documentation for this function. when does it get called? how often does it get called? etc. http://codereview.chromium.org/2862031/diff/31001/32006#newcode16 chrome/renderer/dom_ui_plugin.h:16: virtual ~DOMUIPluginDelegate() { } can this dtor be protected? if it is not protected, then it suggests that the DOMUIPlugin takes ownership of the DOMUIPluginDelegate, but that's probably not what you intend. http://codereview.chromium.org/2862031/diff/31001/32006#newcode19 chrome/renderer/dom_ui_plugin.h:19: class DOMUIPlugin: public WebKit::WebPlugin, public WebKit::WebViewClient, nit: given that this class doesn't technically work with chrome:// URLs, I think it might be confusing to use "DOM UI" in its name. maybe WebViewPlugin would be better. also, maybe this should live in webkit/glue since it is not specific to chrome. http://codereview.chromium.org/2862031/diff/31001/32006#newcode22 chrome/renderer/dom_ui_plugin.h:22: explicit DOMUIPlugin(DOMUIPluginDelegate* client); nit: client -> delegate http://codereview.chromium.org/2862031/diff/31001/32006#newcode23 chrome/renderer/dom_ui_plugin.h:23: virtual ~DOMUIPlugin(); i think this dtor should be made private. you'll probably need to make the DeleteTask (from message_loop.h) be a friend. http://codereview.chromium.org/2862031/diff/31001/32006#newcode70 chrome/renderer/dom_ui_plugin.h:70: const WebKit::WebContextMenuData& data) { } it seems like you can just leave this method out and rely on the default implementation. same goes for other client methods implemented as no-ops here. http://codereview.chromium.org/2862031/diff/31001/32006#newcode92 chrome/renderer/dom_ui_plugin.h:92: nit: i recommend eliminating the new lines between these member variables. http://codereview.chromium.org/2862031/diff/31001/32007 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/2862031/diff/31001/32007#newcode2224 chrome/renderer/render_view.cc:2224: } else { nit: no need for "else" after "return" http://codereview.chromium.org/2862031/diff/31001/32007#newcode3687 chrome/renderer/render_view.cc:3687: webkit_preferences_.Apply(web_view); nit: i'd do away with the web_view temp var and just write: webkit_preferences_.Apply(plugin->web_view()); http://codereview.chromium.org/2862031/diff/31001/32010 File chrome/renderer/resources/blocked_plugin.html (right): http://codereview.chromium.org/2862031/diff/31001/32010#newcode52 chrome/renderer/resources/blocked_plugin.html:52: <div><img src="../../app/theme/extensions_section.png" /></div> how is this image url loaded?
On 2010/07/15 23:55:51, darin wrote: > really sorry... this fell off my radar :( > > http://codereview.chromium.org/2862031/diff/31001/32003 > File chrome/renderer/blocked_plugin.cc (right): > > http://codereview.chromium.org/2862031/diff/31001/32003#newcode48 > chrome/renderer/blocked_plugin.cc:48: NOTREACHED() << "unable to load template. > ID: " << resource_id; > can this just be a DCHECK? as is, if this were to fail in a release build, you > would just have a silent return. maybe a CHECK could be used if you think there > is a possibility of this failing in a production build. the early return just > seems like a good way to have a mysterious failure in a production build, and > doesn't add anything to a debug build. NOTREACHED is #defined to DCHECK(FALSE) anyway, and DCHECK is supposed to be used for recoverable errors, right? I wouldn't want to crash the whole renderer if this should actually fail. > http://codereview.chromium.org/2862031/diff/31001/32003#newcode62 > chrome/renderer/blocked_plugin.cc:62: WebURL(), false); > nit: you can just leave off the optional 3rd and 4th parameters. done. > http://codereview.chromium.org/2862031/diff/31001/32003#newcode75 > chrome/renderer/blocked_plugin.cc:75: CHECK(plugin_); > can this be a DCHECK? if plugin_ is null, i think you'll get a crash report on > the next line, dereferencing container. Hm. Now that I think about it, there shouldn't be a case where |plugin_| is null anyway. > http://codereview.chromium.org/2862031/diff/31001/32003#newcode77 > chrome/renderer/blocked_plugin.cc:77: WebPlugin* newPlugin = > nit: newPlugin -> new_plugin done. > http://codereview.chromium.org/2862031/diff/31001/32005 > File chrome/renderer/dom_ui_plugin.cc (right): > > http://codereview.chromium.org/2862031/diff/31001/32005#newcode62 > chrome/renderer/dom_ui_plugin.cc:62: gfx::Rect paintRect(rect_.Intersect(rect)); > nit: paintRect -> paint_rect done. > http://codereview.chromium.org/2862031/diff/31001/32005#newcode66 > chrome/renderer/dom_ui_plugin.cc:66: paintRect.Offset(-rect_.x(), -rect_.y()); > does all of this transformation stuff work properly when the plugin is embedded > within an iframe? what about if the containing page is scrolled and/or the > iframe is scrolled? Surprisingly enough, it does :-) > http://codereview.chromium.org/2862031/diff/31001/32005#newcode73 > chrome/renderer/dom_ui_plugin.cc:73: skia::PlatformCanvas* platformCanvas = > canvas; > nit: platformCanvas -> platform_canvas done. > http://codereview.chromium.org/2862031/diff/31001/32005#newcode78 > chrome/renderer/dom_ui_plugin.cc:78: if (needs_layout_) { > the WebView may require layout for internal reasons. you have to call layout > before paint. webkit has a dirty bit system, so layout will be a no-op if there > is no work to be done. Oh, nice. Done. > http://codereview.chromium.org/2862031/diff/31001/32005#newcode108 > chrome/renderer/dom_ui_plugin.cc:108: cursor_info_ = &cursor; > hmm... what if didChangeCursor happens outside the context of handleInputEvent? > i'd recommend just having a WebCursorInfo member, call it current_cursor_, and > then at the end of this function set cursor = current_cursor_ before returning. > then in didChangeCursor just set current_cursor_. Done. It probably won't change much, because I still need to wait for the next handleInputEvent call to set the cursor. But at least it simplifies the code. > http://codereview.chromium.org/2862031/diff/31001/32005#newcode146 > chrome/renderer/dom_ui_plugin.cc:146: return url.SchemeIs("chrome"); > nit: return GURL(request.url()).SchemeIs("chrome"); done. > http://codereview.chromium.org/2862031/diff/31001/32005#newcode153 > chrome/renderer/dom_ui_plugin.cc:153: WebURLError error; > you should also initialize the domain member, perhaps to a value like > "DOMUIPlugin" (or "WebViewPlugin" if the class is renamed). done. > http://codereview.chromium.org/2862031/diff/31001/32006 > File chrome/renderer/dom_ui_plugin.h (right): > > http://codereview.chromium.org/2862031/diff/31001/32006#newcode15 > chrome/renderer/dom_ui_plugin.h:15: virtual void BindWebFrame(WebKit::WebFrame* > frame) = 0; > nit: please add some documentation for this function. when does it get called? > how often does it get called? etc. done. > http://codereview.chromium.org/2862031/diff/31001/32006#newcode16 > chrome/renderer/dom_ui_plugin.h:16: virtual ~DOMUIPluginDelegate() { } > can this dtor be protected? if it is not protected, then it suggests that the > DOMUIPlugin takes ownership of the DOMUIPluginDelegate, but that's probably not > what you intend. That *is* what I intend. There are two ways that the plugin and its delegate can be destroyed: by calling the Load callback on the delegate, and by calling Destroy on the plugin. Because WebPlugin::Destroy is already public, I chose to call it from BlockedPlugin::Load and have the plugin then destroy its delegate. > http://codereview.chromium.org/2862031/diff/31001/32006#newcode19 > chrome/renderer/dom_ui_plugin.h:19: class DOMUIPlugin: public WebKit::WebPlugin, > public WebKit::WebViewClient, > nit: given that this class doesn't technically work with chrome:// URLs, I think > it might be confusing to use "DOM UI" in its name. maybe WebViewPlugin would be > better. also, maybe this should live in webkit/glue since it is not specific to > chrome. Okay, done. I also renamed the WebViewPluginDelegate to WebViewPlugin::Delegate to clean the names up a bit. > http://codereview.chromium.org/2862031/diff/31001/32006#newcode22 > chrome/renderer/dom_ui_plugin.h:22: explicit DOMUIPlugin(DOMUIPluginDelegate* > client); > nit: client -> delegate done. > http://codereview.chromium.org/2862031/diff/31001/32006#newcode23 > chrome/renderer/dom_ui_plugin.h:23: virtual ~DOMUIPlugin(); > i think this dtor should be made private. you'll probably need to make the > DeleteTask (from message_loop.h) be a friend. done. > http://codereview.chromium.org/2862031/diff/31001/32006#newcode70 > chrome/renderer/dom_ui_plugin.h:70: const WebKit::WebContextMenuData& data) { } > it seems like you can just leave this method out and rely on the default > implementation. same goes for other client methods implemented as no-ops here. Right, done. > http://codereview.chromium.org/2862031/diff/31001/32006#newcode92 > chrome/renderer/dom_ui_plugin.h:92: > nit: i recommend eliminating the new lines between these member variables. > > http://codereview.chromium.org/2862031/diff/31001/32007 > File chrome/renderer/render_view.cc (right): > > http://codereview.chromium.org/2862031/diff/31001/32007#newcode2224 > chrome/renderer/render_view.cc:2224: } else { > nit: no need for "else" after "return" > > http://codereview.chromium.org/2862031/diff/31001/32007#newcode3687 > chrome/renderer/render_view.cc:3687: webkit_preferences_.Apply(web_view); > nit: i'd do away with the web_view temp var and just write: > > webkit_preferences_.Apply(plugin->web_view()); All done. > http://codereview.chromium.org/2862031/diff/31001/32010 > File chrome/renderer/resources/blocked_plugin.html (right): > > http://codereview.chromium.org/2862031/diff/31001/32010#newcode52 > chrome/renderer/resources/blocked_plugin.html:52: <div><img > src="../../app/theme/extensions_section.png" /></div> > how is this image url loaded? It's actually replaced with a data: URL containing the file contents during build. Neat, right?
On Fri, Jul 16, 2010 at 8:15 AM, <bauerb@chromium.org> wrote: > On 2010/07/15 23:55:51, darin wrote: > >> really sorry... this fell off my radar :( >> > > http://codereview.chromium.org/2862031/diff/31001/32003 >> File chrome/renderer/blocked_plugin.cc (right): >> > > http://codereview.chromium.org/2862031/diff/31001/32003#newcode48 >> chrome/renderer/blocked_plugin.cc:48: NOTREACHED() << "unable to load >> > template. > >> ID: " << resource_id; >> can this just be a DCHECK? as is, if this were to fail in a release >> build, >> > you > >> would just have a silent return. maybe a CHECK could be used if you think >> > there > >> is a possibility of this failing in a production build. the early return >> just >> seems like a good way to have a mysterious failure in a production build, >> and >> doesn't add anything to a debug build. >> > > NOTREACHED is #defined to DCHECK(FALSE) anyway, and DCHECK is supposed to > be > used for recoverable errors, right? I wouldn't want to crash the whole > renderer > if this should actually fail. > > See http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE... Is this an error that can actually happen during production assuming we didn't forget to package up the resource? If such were to happen, we'd want to phone home about it. Crashing is the only way to do that presently. > > http://codereview.chromium.org/2862031/diff/31001/32003#newcode62 >> chrome/renderer/blocked_plugin.cc:62: WebURL(), false); >> nit: you can just leave off the optional 3rd and 4th parameters. >> > > done. > > http://codereview.chromium.org/2862031/diff/31001/32003#newcode75 >> chrome/renderer/blocked_plugin.cc:75: CHECK(plugin_); >> can this be a DCHECK? if plugin_ is null, i think you'll get a crash >> report >> > on > >> the next line, dereferencing container. >> > > Hm. Now that I think about it, there shouldn't be a case where |plugin_| is > null > anyway. > > > http://codereview.chromium.org/2862031/diff/31001/32003#newcode77 >> chrome/renderer/blocked_plugin.cc:77: WebPlugin* newPlugin = >> nit: newPlugin -> new_plugin >> > > done. > > http://codereview.chromium.org/2862031/diff/31001/32005 >> File chrome/renderer/dom_ui_plugin.cc (right): >> > > http://codereview.chromium.org/2862031/diff/31001/32005#newcode62 >> chrome/renderer/dom_ui_plugin.cc:62: gfx::Rect >> > paintRect(rect_.Intersect(rect)); > >> nit: paintRect -> paint_rect >> > > done. > > http://codereview.chromium.org/2862031/diff/31001/32005#newcode66 >> chrome/renderer/dom_ui_plugin.cc:66: paintRect.Offset(-rect_.x(), >> -rect_.y()); >> does all of this transformation stuff work properly when the plugin is >> > embedded > >> within an iframe? what about if the containing page is scrolled and/or >> the >> iframe is scrolled? >> > > Surprisingly enough, it does :-) Great :) > > > http://codereview.chromium.org/2862031/diff/31001/32005#newcode73 >> chrome/renderer/dom_ui_plugin.cc:73: skia::PlatformCanvas* platformCanvas >> = >> canvas; >> nit: platformCanvas -> platform_canvas >> > > done. > > http://codereview.chromium.org/2862031/diff/31001/32005#newcode78 >> chrome/renderer/dom_ui_plugin.cc:78: if (needs_layout_) { >> the WebView may require layout for internal reasons. you have to call >> layout >> before paint. webkit has a dirty bit system, so layout will be a no-op if >> > there > >> is no work to be done. >> > > Oh, nice. Done. > > > http://codereview.chromium.org/2862031/diff/31001/32005#newcode108 >> chrome/renderer/dom_ui_plugin.cc:108: cursor_info_ = &cursor; >> hmm... what if didChangeCursor happens outside the context of >> > handleInputEvent? > >> i'd recommend just having a WebCursorInfo member, call it current_cursor_, >> and >> then at the end of this function set cursor = current_cursor_ before >> > returning. > >> then in didChangeCursor just set current_cursor_. >> > > Done. It probably won't change much, because I still need to wait for the > next > handleInputEvent call to set the cursor. But at least it simplifies the > code. > > > http://codereview.chromium.org/2862031/diff/31001/32005#newcode146 >> chrome/renderer/dom_ui_plugin.cc:146: return url.SchemeIs("chrome"); >> nit: return GURL(request.url()).SchemeIs("chrome"); >> > > done. > > http://codereview.chromium.org/2862031/diff/31001/32005#newcode153 >> chrome/renderer/dom_ui_plugin.cc:153: WebURLError error; >> you should also initialize the domain member, perhaps to a value like >> "DOMUIPlugin" (or "WebViewPlugin" if the class is renamed). >> > > done. > > http://codereview.chromium.org/2862031/diff/31001/32006 >> File chrome/renderer/dom_ui_plugin.h (right): >> > > http://codereview.chromium.org/2862031/diff/31001/32006#newcode15 >> chrome/renderer/dom_ui_plugin.h:15: virtual void >> > BindWebFrame(WebKit::WebFrame* > >> frame) = 0; >> nit: please add some documentation for this function. when does it get >> > called? > >> how often does it get called? etc. >> > > done. > > http://codereview.chromium.org/2862031/diff/31001/32006#newcode16 >> chrome/renderer/dom_ui_plugin.h:16: virtual ~DOMUIPluginDelegate() { } >> can this dtor be protected? if it is not protected, then it suggests that >> the >> DOMUIPlugin takes ownership of the DOMUIPluginDelegate, but that's >> probably >> > not > >> what you intend. >> > > That *is* what I intend. There are two ways that the plugin and its > delegate can > be destroyed: by calling the Load callback on the delegate, and by calling > Destroy on the plugin. Because WebPlugin::Destroy is already public, I > chose to > call it from BlockedPlugin::Load and have the plugin then destroy its > delegate. > > Oh... in Chrome it is very unusual for a delegate to be deleted like that. Normally, the delegate is implemented by the owner of the object that we set the delegate on. I can see now why you would want the delegate to be owned by the plugin since the plugin is kept alive by the WebPluginContainer. A typical solution is to add a notification on the Delegate to indicate that the WebViewPlugin will be or has been destroyed (Will/DidDestroyPlugin). The BlockedPlugin can then observe this notification, and determine that it should delete itself. > > http://codereview.chromium.org/2862031/diff/31001/32006#newcode19 >> chrome/renderer/dom_ui_plugin.h:19: class DOMUIPlugin: public >> > WebKit::WebPlugin, > >> public WebKit::WebViewClient, >> nit: given that this class doesn't technically work with chrome:// URLs, I >> > think > >> it might be confusing to use "DOM UI" in its name. maybe WebViewPlugin >> would >> > be > >> better. also, maybe this should live in webkit/glue since it is not >> specific >> > to > >> chrome. >> > > Okay, done. I also renamed the WebViewPluginDelegate to > WebViewPlugin::Delegate > to clean the names up a bit. > > Good idea. > > http://codereview.chromium.org/2862031/diff/31001/32006#newcode22 >> chrome/renderer/dom_ui_plugin.h:22: explicit >> DOMUIPlugin(DOMUIPluginDelegate* >> client); >> nit: client -> delegate >> > > done. > > http://codereview.chromium.org/2862031/diff/31001/32006#newcode23 >> chrome/renderer/dom_ui_plugin.h:23: virtual ~DOMUIPlugin(); >> i think this dtor should be made private. you'll probably need to make >> the >> DeleteTask (from message_loop.h) be a friend. >> > > done. > > http://codereview.chromium.org/2862031/diff/31001/32006#newcode70 >> chrome/renderer/dom_ui_plugin.h:70: const WebKit::WebContextMenuData& >> data) { >> > } > >> it seems like you can just leave this method out and rely on the default >> implementation. same goes for other client methods implemented as no-ops >> > here. > > Right, done. > > > http://codereview.chromium.org/2862031/diff/31001/32006#newcode92 >> chrome/renderer/dom_ui_plugin.h:92: >> nit: i recommend eliminating the new lines between these member variables. >> > > http://codereview.chromium.org/2862031/diff/31001/32007 >> File chrome/renderer/render_view.cc (right): >> > > http://codereview.chromium.org/2862031/diff/31001/32007#newcode2224 >> chrome/renderer/render_view.cc:2224: } else { >> nit: no need for "else" after "return" >> > > http://codereview.chromium.org/2862031/diff/31001/32007#newcode3687 >> chrome/renderer/render_view.cc:3687: webkit_preferences_.Apply(web_view); >> nit: i'd do away with the web_view temp var and just write: >> > > webkit_preferences_.Apply(plugin->web_view()); >> > > All done. > > > http://codereview.chromium.org/2862031/diff/31001/32010 >> File chrome/renderer/resources/blocked_plugin.html (right): >> > > http://codereview.chromium.org/2862031/diff/31001/32010#newcode52 >> chrome/renderer/resources/blocked_plugin.html:52: <div><img >> src="../../app/theme/extensions_section.png" /></div> >> how is this image url loaded? >> > > It's actually replaced with a data: URL containing the file contents during > build. Neat, right? > That is quite neat :) > > http://codereview.chromium.org/2862031/show >
On Fri, Jul 16, 2010 at 8:51 AM, Darin Fisher <darin@chromium.org> wrote: > On Fri, Jul 16, 2010 at 8:15 AM, <bauerb@chromium.org> wrote: > >> On 2010/07/15 23:55:51, darin wrote: >> >>> really sorry... this fell off my radar :( >>> >> >> http://codereview.chromium.org/2862031/diff/31001/32003 >>> File chrome/renderer/blocked_plugin.cc (right): >>> >> >> http://codereview.chromium.org/2862031/diff/31001/32003#newcode48 >>> chrome/renderer/blocked_plugin.cc:48: NOTREACHED() << "unable to load >>> >> template. >> >>> ID: " << resource_id; >>> can this just be a DCHECK? as is, if this were to fail in a release >>> build, >>> >> you >> >>> would just have a silent return. maybe a CHECK could be used if you >>> think >>> >> there >> >>> is a possibility of this failing in a production build. the early return >>> just >>> seems like a good way to have a mysterious failure in a production build, >>> and >>> doesn't add anything to a debug build. >>> >> >> NOTREACHED is #defined to DCHECK(FALSE) anyway, and DCHECK is supposed to >> be >> used for recoverable errors, right? I wouldn't want to crash the whole >> renderer >> if this should actually fail. >> >> > See > http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE... > > Is this an error that can actually happen during production assuming we > didn't > forget to package up the resource? If such were to happen, we'd want to > phone > home about it. Crashing is the only way to do that presently. > > > >> >> http://codereview.chromium.org/2862031/diff/31001/32003#newcode62 >>> chrome/renderer/blocked_plugin.cc:62: WebURL(), false); >>> nit: you can just leave off the optional 3rd and 4th parameters. >>> >> >> done. >> >> http://codereview.chromium.org/2862031/diff/31001/32003#newcode75 >>> chrome/renderer/blocked_plugin.cc:75: CHECK(plugin_); >>> can this be a DCHECK? if plugin_ is null, i think you'll get a crash >>> report >>> >> on >> >>> the next line, dereferencing container. >>> >> >> Hm. Now that I think about it, there shouldn't be a case where |plugin_| >> is null >> anyway. >> >> >> http://codereview.chromium.org/2862031/diff/31001/32003#newcode77 >>> chrome/renderer/blocked_plugin.cc:77: WebPlugin* newPlugin = >>> nit: newPlugin -> new_plugin >>> >> >> done. >> >> http://codereview.chromium.org/2862031/diff/31001/32005 >>> File chrome/renderer/dom_ui_plugin.cc (right): >>> >> >> http://codereview.chromium.org/2862031/diff/31001/32005#newcode62 >>> chrome/renderer/dom_ui_plugin.cc:62: gfx::Rect >>> >> paintRect(rect_.Intersect(rect)); >> >>> nit: paintRect -> paint_rect >>> >> >> done. >> >> http://codereview.chromium.org/2862031/diff/31001/32005#newcode66 >>> chrome/renderer/dom_ui_plugin.cc:66: paintRect.Offset(-rect_.x(), >>> -rect_.y()); >>> does all of this transformation stuff work properly when the plugin is >>> >> embedded >> >>> within an iframe? what about if the containing page is scrolled and/or >>> the >>> iframe is scrolled? >>> >> >> Surprisingly enough, it does :-) > > > Great :) > > >> >> >> http://codereview.chromium.org/2862031/diff/31001/32005#newcode73 >>> chrome/renderer/dom_ui_plugin.cc:73: skia::PlatformCanvas* platformCanvas >>> = >>> canvas; >>> nit: platformCanvas -> platform_canvas >>> >> >> done. >> >> http://codereview.chromium.org/2862031/diff/31001/32005#newcode78 >>> chrome/renderer/dom_ui_plugin.cc:78: if (needs_layout_) { >>> the WebView may require layout for internal reasons. you have to call >>> layout >>> before paint. webkit has a dirty bit system, so layout will be a no-op >>> if >>> >> there >> >>> is no work to be done. >>> >> >> Oh, nice. Done. >> >> >> http://codereview.chromium.org/2862031/diff/31001/32005#newcode108 >>> chrome/renderer/dom_ui_plugin.cc:108: cursor_info_ = &cursor; >>> hmm... what if didChangeCursor happens outside the context of >>> >> handleInputEvent? >> >>> i'd recommend just having a WebCursorInfo member, call it >>> current_cursor_, and >>> then at the end of this function set cursor = current_cursor_ before >>> >> returning. >> >>> then in didChangeCursor just set current_cursor_. >>> >> >> Done. It probably won't change much, because I still need to wait for the >> next >> handleInputEvent call to set the cursor. But at least it simplifies the >> code. >> >> >> http://codereview.chromium.org/2862031/diff/31001/32005#newcode146 >>> chrome/renderer/dom_ui_plugin.cc:146: return url.SchemeIs("chrome"); >>> nit: return GURL(request.url()).SchemeIs("chrome"); >>> >> >> done. >> >> http://codereview.chromium.org/2862031/diff/31001/32005#newcode153 >>> chrome/renderer/dom_ui_plugin.cc:153: WebURLError error; >>> you should also initialize the domain member, perhaps to a value like >>> "DOMUIPlugin" (or "WebViewPlugin" if the class is renamed). >>> >> >> done. >> >> http://codereview.chromium.org/2862031/diff/31001/32006 >>> File chrome/renderer/dom_ui_plugin.h (right): >>> >> >> http://codereview.chromium.org/2862031/diff/31001/32006#newcode15 >>> chrome/renderer/dom_ui_plugin.h:15: virtual void >>> >> BindWebFrame(WebKit::WebFrame* >> >>> frame) = 0; >>> nit: please add some documentation for this function. when does it get >>> >> called? >> >>> how often does it get called? etc. >>> >> >> done. >> >> http://codereview.chromium.org/2862031/diff/31001/32006#newcode16 >>> chrome/renderer/dom_ui_plugin.h:16: virtual ~DOMUIPluginDelegate() { } >>> can this dtor be protected? if it is not protected, then it suggests >>> that the >>> DOMUIPlugin takes ownership of the DOMUIPluginDelegate, but that's >>> probably >>> >> not >> >>> what you intend. >>> >> >> That *is* what I intend. There are two ways that the plugin and its >> delegate can >> be destroyed: by calling the Load callback on the delegate, and by calling >> Destroy on the plugin. Because WebPlugin::Destroy is already public, I >> chose to >> call it from BlockedPlugin::Load and have the plugin then destroy its >> delegate. >> >> > Oh... in Chrome it is very unusual for a delegate to be deleted like that. > Normally, > the delegate is implemented by the owner of the object that we set the > delegate on. > > I can see now why you would want the delegate to be owned by the plugin > since > the plugin is kept alive by the WebPluginContainer. > > A typical solution is to add a notification on the Delegate to indicate > that the > WebViewPlugin will be or has been destroyed (Will/DidDestroyPlugin). The > BlockedPlugin can then observe this notification, and determine that it > should > delete itself. > > Finally, another option is to make BlockedPlugin extend from WebViewPlugin. This could make sense in this case since the BlockedPlugin allocates the WebViewPlugin in its constructor. Some would argue that composition is better than inheritance, which leads us to the Delegate solution, but I'd be OK with inheritance in this case if it cleans things up. Not sure which I would do at this point... -Darin > > > >> >> http://codereview.chromium.org/2862031/diff/31001/32006#newcode19 >>> chrome/renderer/dom_ui_plugin.h:19: class DOMUIPlugin: public >>> >> WebKit::WebPlugin, >> >>> public WebKit::WebViewClient, >>> nit: given that this class doesn't technically work with chrome:// URLs, >>> I >>> >> think >> >>> it might be confusing to use "DOM UI" in its name. maybe WebViewPlugin >>> would >>> >> be >> >>> better. also, maybe this should live in webkit/glue since it is not >>> specific >>> >> to >> >>> chrome. >>> >> >> Okay, done. I also renamed the WebViewPluginDelegate to >> WebViewPlugin::Delegate >> to clean the names up a bit. >> >> > Good idea. > > > >> >> http://codereview.chromium.org/2862031/diff/31001/32006#newcode22 >>> chrome/renderer/dom_ui_plugin.h:22: explicit >>> DOMUIPlugin(DOMUIPluginDelegate* >>> client); >>> nit: client -> delegate >>> >> >> done. >> >> http://codereview.chromium.org/2862031/diff/31001/32006#newcode23 >>> chrome/renderer/dom_ui_plugin.h:23: virtual ~DOMUIPlugin(); >>> i think this dtor should be made private. you'll probably need to make >>> the >>> DeleteTask (from message_loop.h) be a friend. >>> >> >> done. >> >> http://codereview.chromium.org/2862031/diff/31001/32006#newcode70 >>> chrome/renderer/dom_ui_plugin.h:70: const WebKit::WebContextMenuData& >>> data) { >>> >> } >> >>> it seems like you can just leave this method out and rely on the default >>> implementation. same goes for other client methods implemented as no-ops >>> >> here. >> >> Right, done. >> >> >> http://codereview.chromium.org/2862031/diff/31001/32006#newcode92 >>> chrome/renderer/dom_ui_plugin.h:92: >>> nit: i recommend eliminating the new lines between these member >>> variables. >>> >> >> http://codereview.chromium.org/2862031/diff/31001/32007 >>> File chrome/renderer/render_view.cc (right): >>> >> >> http://codereview.chromium.org/2862031/diff/31001/32007#newcode2224 >>> chrome/renderer/render_view.cc:2224: } else { >>> nit: no need for "else" after "return" >>> >> >> http://codereview.chromium.org/2862031/diff/31001/32007#newcode3687 >>> chrome/renderer/render_view.cc:3687: webkit_preferences_.Apply(web_view); >>> nit: i'd do away with the web_view temp var and just write: >>> >> >> webkit_preferences_.Apply(plugin->web_view()); >>> >> >> All done. >> >> >> http://codereview.chromium.org/2862031/diff/31001/32010 >>> File chrome/renderer/resources/blocked_plugin.html (right): >>> >> >> http://codereview.chromium.org/2862031/diff/31001/32010#newcode52 >>> chrome/renderer/resources/blocked_plugin.html:52: <div><img >>> src="../../app/theme/extensions_section.png" /></div> >>> how is this image url loaded? >>> >> >> It's actually replaced with a data: URL containing the file contents >> during >> build. Neat, right? >> > > That is quite neat :) > > >> >> http://codereview.chromium.org/2862031/show >> > >
On 2010/07/16 15:51:42, darin wrote: > See > http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE... > > Is this an error that can actually happen during production assuming we > didn't > forget to package up the resource? If such were to happen, we'd want to > phone > home about it. Crashing is the only way to do that presently. I don't know how this should happen if we bundle all resources. To be honest, I copied that code from RenderView::GetAltHTMLForTemplate. Which gets the resource ID as a parameter, I see now, so it does make more sense to check if it's valid there than it does here. So, what's your proposal here? (On a tangent, it'd be nice nevertheless to have a way to phone home about things that aren't supposed to happen, while still being able to handle them gracefully) > I can see now why you would want the delegate to be owned by the plugin > since > the plugin is kept alive by the WebPluginContainer. > > A typical solution is to add a notification on the Delegate to indicate that > the > WebViewPlugin will be or has been destroyed (Will/DidDestroyPlugin). The > BlockedPlugin can then observe this notification, and determine that it > should > delete itself. Sounds good. > Finally, another option is to make BlockedPlugin extend from WebViewPlugin. > This could make sense in this case since the BlockedPlugin allocates the > WebViewPlugin in its constructor. > > Some would argue that composition is better than inheritance, which leads us > to the Delegate solution, but I'd be OK with inheritance in this case if it > cleans > things up. I'd thought about that too, but BlockedPlugin already needs to inherit from CppBoundClass for the Javascript callbacks, and I wanted to avoid multiple inheritance. But maybe I could have WebViewPlugin directly inherit from CppBoundClass? Registering for Javascript callbacks could very well be used by hypothetical other subclasses as well. WDYT?
On Fri, Jul 16, 2010 at 9:35 AM, <bauerb@chromium.org> wrote: > On 2010/07/16 15:51:42, darin wrote: > >> See >> > > > http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE... > > Is this an error that can actually happen during production assuming we >> didn't >> forget to package up the resource? If such were to happen, we'd want to >> phone >> home about it. Crashing is the only way to do that presently. >> > > I don't know how this should happen if we bundle all resources. To be > honest, I > copied that code from RenderView::GetAltHTMLForTemplate. Yeah... we don't get this right in a number of cases. Granted, this is a minor issue, but I figured it was worth pointing out. > Which gets the resource > ID as a parameter, I see now, so it does make more sense to check if it's > valid > there than it does here. > > So, what's your proposal here? > Just DCHECK and don't bother with handling the error at runtime. Assume it is developer error to forget to package the resource, and so they should get an assertion failure followed by a crash. Seems like that will be enough for developers to realize their error and fix during development. > > (On a tangent, it'd be nice nevertheless to have a way to phone home about > things that aren't supposed to happen, while still being able to handle > them > gracefully) Agreed. Breakpad supports that. We just don't have a convenient way of poking it AFAIK. > > > I can see now why you would want the delegate to be owned by the plugin >> since >> the plugin is kept alive by the WebPluginContainer. >> > > A typical solution is to add a notification on the Delegate to indicate >> that >> the >> WebViewPlugin will be or has been destroyed (Will/DidDestroyPlugin). The >> BlockedPlugin can then observe this notification, and determine that it >> should >> delete itself. >> > > Sounds good. > > > Finally, another option is to make BlockedPlugin extend from >> WebViewPlugin. >> This could make sense in this case since the BlockedPlugin allocates the >> WebViewPlugin in its constructor. >> > > Some would argue that composition is better than inheritance, which leads >> us >> to the Delegate solution, but I'd be OK with inheritance in this case if >> it >> cleans >> things up. >> > > I'd thought about that too, but BlockedPlugin already needs to inherit from > CppBoundClass for the Javascript callbacks, and I wanted to avoid multiple > inheritance. > > But maybe I could have WebViewPlugin directly inherit from CppBoundClass? > Registering for Javascript callbacks could very well be used by > hypothetical > other subclasses as well. WDYT? > > Yeah, the multiple inheritance is a little unfortunate. I'd probably just go with the destroy callback then :-/ -Darin > > http://codereview.chromium.org/2862031/show >
Okay, done and done. (Seeing as the CL seems to be converging (and the feature freeze approaching), could you give something like "LGTM with nits" when there are only nits left, so I can land this on Monday morning?) Bernhard. On 2010/07/16 17:12:50, darin wrote: > On Fri, Jul 16, 2010 at 9:35 AM, <mailto:bauerb@chromium.org> wrote: > > > On 2010/07/16 15:51:42, darin wrote: > > > >> See > >> > > > > > > > http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE... > > > > Is this an error that can actually happen during production assuming we > >> didn't > >> forget to package up the resource? If such were to happen, we'd want to > >> phone > >> home about it. Crashing is the only way to do that presently. > >> > > > > I don't know how this should happen if we bundle all resources. To be > > honest, I > > copied that code from RenderView::GetAltHTMLForTemplate. > > > Yeah... we don't get this right in a number of cases. Granted, this is a > minor > issue, but I figured it was worth pointing out. > > > > > Which gets the resource > > ID as a parameter, I see now, so it does make more sense to check if it's > > valid > > there than it does here. > > > > So, what's your proposal here? > > > > Just DCHECK and don't bother with handling the error at runtime. Assume it > is developer error to forget to package the resource, and so they should get > an assertion failure followed by a crash. Seems like that will be enough > for > developers to realize their error and fix during development. > > > > > > (On a tangent, it'd be nice nevertheless to have a way to phone home about > > things that aren't supposed to happen, while still being able to handle > > them > > gracefully) > > > Agreed. Breakpad supports that. We just don't have a convenient way of > poking it AFAIK. > > > > > > > > > I can see now why you would want the delegate to be owned by the plugin > >> since > >> the plugin is kept alive by the WebPluginContainer. > >> > > > > A typical solution is to add a notification on the Delegate to indicate > >> that > >> the > >> WebViewPlugin will be or has been destroyed (Will/DidDestroyPlugin). The > >> BlockedPlugin can then observe this notification, and determine that it > >> should > >> delete itself. > >> > > > > Sounds good. > > > > > > Finally, another option is to make BlockedPlugin extend from > >> WebViewPlugin. > >> This could make sense in this case since the BlockedPlugin allocates the > >> WebViewPlugin in its constructor. > >> > > > > Some would argue that composition is better than inheritance, which leads > >> us > >> to the Delegate solution, but I'd be OK with inheritance in this case if > >> it > >> cleans > >> things up. > >> > > > > I'd thought about that too, but BlockedPlugin already needs to inherit from > > CppBoundClass for the Javascript callbacks, and I wanted to avoid multiple > > inheritance. > > > > But maybe I could have WebViewPlugin directly inherit from CppBoundClass? > > Registering for Javascript callbacks could very well be used by > > hypothetical > > other subclasses as well. WDYT? > > > > > Yeah, the multiple inheritance is a little unfortunate. I'd probably just > go with > the destroy callback then :-/ > > -Darin > > > > > > http://codereview.chromium.org/2862031/show > > >
LGTM w/ nits: http://codereview.chromium.org/2862031/diff/47001/44006 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/2862031/diff/47001/44006#newcode3741 chrome/renderer/render_view.cc:3741: BlockedPlugin* blocked_plugin = new BlockedPlugin(this, frame, params); You should probably add a note here about the fact that the BlockedPlugin instance will be destroyed when the WebViewPlugin is destroyed. Otherwise, from reading this code, it really looks like a memory leak. http://codereview.chromium.org/2862031/diff/47001/44010 File webkit/glue/plugins/webview_plugin.cc (right): http://codereview.chromium.org/2862031/diff/47001/44010#newcode56 webkit/glue/plugins/webview_plugin.cc:56: delegate_->WillDestroyPlugin(); nit: i recommend nulling out the delegate_ here (after WillDestroyPlugin) just in case we happen to reach code that tries to call the delegate_ again. tracking down a null pointer deref is easier than a deref of garbage memory. http://codereview.chromium.org/2862031/diff/47001/44011 File webkit/glue/plugins/webview_plugin.h (right): http://codereview.chromium.org/2862031/diff/47001/44011#newcode14 webkit/glue/plugins/webview_plugin.h:14: nit: add a descriptive comment explaining what this class is and how it may be used, etc.
One conceptual question (doesn't block this CL): Some sites check navigator.plugins and don't even put an object/embed tag on the page if flash isn't in that list. When plugins are disabled, chrome sets navigator.plugins to an empty list. Hence, on these pages, click-to-plugin wouldn't work since there would be no plugin. Is the plan to not set navigator.plugins to an empty list when plugins are disabled?
Yeah, I think we need to undo that behavior. We should make it look like the plugins exist. I know this breaks some sites if we then do not actually let the plugins load, but that might be the lesser of the two issues. -Darin On Sun, Jul 18, 2010 at 10:13 PM, <thakis@chromium.org> wrote: > One conceptual question (doesn't block this CL): Some sites check > navigator.plugins and don't even put an object/embed tag on the page if > flash > isn't in that list. When plugins are disabled, chrome sets > navigator.plugins to > an empty list. Hence, on these pages, click-to-plugin wouldn't work since > there > would be no plugin. Is the plan to not set navigator.plugins to an empty > list > when plugins are disabled? > > > > http://codereview.chromium.org/2862031/show >
That's actually already a side effect of this CL (otherwise we wouldn't get the necessary data to load the plugin later on in RenderView::CreatePlugin), achieved by using the default behavior for RenderView::AllowPlugins (before that we used to check content settings there, which we now do in CreatePlugin). On Mon, Jul 19, 2010 at 07:43, Darin Fisher <darin@chromium.org> wrote: > Yeah, I think we need to undo that behavior. We should make it look like > the plugins > exist. I know this breaks some sites if we then do not actually let the > plugins load, > but that might be the lesser of the two issues. > -Darin > > On Sun, Jul 18, 2010 at 10:13 PM, <thakis@chromium.org> wrote: >> >> One conceptual question (doesn't block this CL): Some sites check >> navigator.plugins and don't even put an object/embed tag on the page if >> flash >> isn't in that list. When plugins are disabled, chrome sets >> navigator.plugins to >> an empty list. Hence, on these pages, click-to-plugin wouldn't work since >> there >> would be no plugin. Is the plan to not set navigator.plugins to an empty >> list >> when plugins are disabled? >> >> >> http://codereview.chromium.org/2862031/show > >
On Sun, Jul 18, 2010 at 10:43 PM, Darin Fisher <darin@chromium.org> wrote: > Yeah, I think we need to undo that behavior. We should make it look like > the plugins > exist. I know this breaks some sites if we then do not actually let the > plugins load, > but that might be the lesser of the two issues. > I haven't paid attention to the UI changes in this review. Is there still a mode where a plugin is disabled and doesn't have click-to-play (it's just permanently off, like today)? If so, then in that mode, we should probably not revert the behavior you describe. For the click-to-play mode, I agree that we should probably revert this. http://codereview.chromium.org/2862031/show >> > >
On 2010/07/19 17:01:17, Peter Kasting wrote: > On Sun, Jul 18, 2010 at 10:43 PM, Darin Fisher <mailto:darin@chromium.org> wrote: > > > Yeah, I think we need to undo that behavior. We should make it look like > > the plugins > > exist. I know this breaks some sites if we then do not actually let the > > plugins load, > > but that might be the lesser of the two issues. > > > > I haven't paid attention to the UI changes in this review. Is there still a > mode where a plugin is disabled and doesn't have click-to-play (it's just > permanently off, like today)? I believe there isn't. > If so, then in that mode, we should probably > not revert the behavior you describe. For the click-to-play mode, I agree > that we should probably revert this. > > http://codereview.chromium.org/2862031/show > >> > > > > >
If a plugin is disabled via about:plugins, then it doesn't show up in navigator.plugins. I can't think of another (non-pathological) case where a plugin doesn't appear and there's also no click-to-play. On Mon, Jul 19, 2010 at 19:00, Peter Kasting <pkasting@chromium.org> wrote: > On Sun, Jul 18, 2010 at 10:43 PM, Darin Fisher <darin@chromium.org> wrote: >> >> Yeah, I think we need to undo that behavior. We should make it look like >> the plugins >> exist. I know this breaks some sites if we then do not actually let the >> plugins load, >> but that might be the lesser of the two issues. > > I haven't paid attention to the UI changes in this review. Is there still a > mode where a plugin is disabled and doesn't have click-to-play (it's just > permanently off, like today)? If so, then in that mode, we should probably > not revert the behavior you describe. For the click-to-play mode, I agree > that we should probably revert this. >>> >>> http://codereview.chromium.org/2862031/show >> > > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
