Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11418261:
Browser Plugin: Update DOM Node attributes when properties are updated (Closed)
|
Created:
8 years ago by Fady Samuel Modified:
8 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBrowser Plugin: Update DOM Node attributes when properties are updated
BUG=163611
Test=BrowserPluginHostTest.*, BrowserPluginTest.*, WebViewTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170866
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171297
Patch Set 1 #
Total comments: 6
Patch Set 2 : Cleanup #
Total comments: 9
Patch Set 3 : Fixed nits. Merged with ToT. #
Total comments: 4
Patch Set 4 : Addressed sadrul@'s comments #Patch Set 5 : Moved comment to the right place #Patch Set 6 : Fixed spacing #
Total comments: 4
Patch Set 7 : Fixed nits #
Messages
Total messages: 17 (0 generated)
Hi Istiaque, Could I please get your thoughts on this? I feel that we should try to keep DOM attributes and javascript properties in sync, and currently we don't do that. This also fixes a bug where we update the src_ attribute on every load commit instead of just top level commits. I could move that to separate CL? I'm not terribly happy with this code. I would like us to keep constants in one place to be shared across multiple files. I'm also not terribly fond of the idea of sprinkling "UpdateAttribute" everywhere, but I can't think of a cleaner way to do this. Thoughts? Fady
https://codereview.chromium.org/11418261/diff/1/content/renderer/browser_plug... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/11418261/diff/1/content/renderer/browser_plug... content/renderer/browser_plugin/browser_plugin.cc:60: const char kAutoSize[] = "autosize"; I'd group them separately if we have to add them, these are not "parameters/properties on events", maybe group them under 'attributes' or sth similar. One way to avoid these dup attribute names would be to make all SetXXXAttribute return bool, and in bindings if the func returns true you can UpdateAttribute from there. e.g: BrowserPluginPropertyBindingMaxHeight { if (bindings->instance()->SetMaxHeightAttribute(max_height)) bindings->instance()->SetAttribute(name_, ...); } You wouldn't need these in that case. WDYT? https://codereview.chromium.org/11418261/diff/1/content/renderer/browser_plug... content/renderer/browser_plugin/browser_plugin.cc:197: UpdateAttribute(kSrc, src_); UpdateDOMAttribute maybe? b/c we already call the internal attribute update functions 'SetXXXAttribute' maybe this will explicitly suggest that it's for DOM element only. https://codereview.chromium.org/11418261/diff/1/content/renderer/browser_plug... content/renderer/browser_plugin/browser_plugin.cc:639: if (params.is_top_level) { Yes, please make this fix in a separate CL.
PTAL https://codereview.chromium.org/11418261/diff/1/content/renderer/browser_plug... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/11418261/diff/1/content/renderer/browser_plug... content/renderer/browser_plugin/browser_plugin.cc:60: const char kAutoSize[] = "autosize"; On 2012/11/30 19:15:06, lazyboy wrote: > I'd group them separately if we have to add them, these are not > "parameters/properties on events", maybe group them under 'attributes' or sth > similar. > > > One way to avoid these dup attribute names would be to make all SetXXXAttribute > return bool, and in bindings if the func returns true you can UpdateAttribute > from there. e.g: > > BrowserPluginPropertyBindingMaxHeight { > if (bindings->instance()->SetMaxHeightAttribute(max_height)) > bindings->instance()->SetAttribute(name_, ...); > } > > You wouldn't need these in that case. WDYT? Added UpdateDOMAttribute as an abstract method of BrowserPluginPropertyBinding. https://codereview.chromium.org/11418261/diff/1/content/renderer/browser_plug... content/renderer/browser_plugin/browser_plugin.cc:197: UpdateAttribute(kSrc, src_); On 2012/11/30 19:15:06, lazyboy wrote: > UpdateDOMAttribute maybe? b/c we already call the internal attribute update > functions 'SetXXXAttribute' maybe this will explicitly suggest that it's for DOM > element only. Done. https://codereview.chromium.org/11418261/diff/1/content/renderer/browser_plug... content/renderer/browser_plugin/browser_plugin.cc:639: if (params.is_top_level) { On 2012/11/30 19:15:06, lazyboy wrote: > Yes, please make this fix in a separate CL. Done. https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:627: if (params.is_top_level) { This is being addressed in a separate patch. However, UpdateDOMAttribute still needs to get called here.
lgtm https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:264: const std::string& attribute_name, nit fits in previous line. https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:627: if (params.is_top_level) { On 2012/11/30 21:56:51, Fady Samuel wrote: > This is being addressed in a separate patch. However, UpdateDOMAttribute still > needs to get called here. Yes https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin_bindings.cc (right): https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin_bindings.cc:407: virtual void UpdateDOMAttribute(BrowserPluginBindings* bindings) = 0; I'd make this non pure virtual, and remove empty impl in ContentWindowBinding below. Also add method comment 'Updates dom attribute value with the current plugin attribute value', to make it clear that we need to call this *after* setting the attribute value to plugin. https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin_bindings.cc:744: return false; nit: is break; more readable? not sure.
Fixed nits. Sadrul, could you please take a look? https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:264: const std::string& attribute_name, On 2012/11/30 22:11:36, lazyboy wrote: > nit fits in previous line. Done. https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:627: if (params.is_top_level) { On 2012/11/30 22:11:36, lazyboy wrote: > On 2012/11/30 21:56:51, Fady Samuel wrote: > > This is being addressed in a separate patch. However, UpdateDOMAttribute still > > needs to get called here. > Yes That change is on the CQ. I will land this change once the other one lands. https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin_bindings.cc (right): https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin_bindings.cc:407: virtual void UpdateDOMAttribute(BrowserPluginBindings* bindings) = 0; On 2012/11/30 22:11:36, lazyboy wrote: > I'd make this non pure virtual, and remove empty impl in ContentWindowBinding > below. > > Also add method comment 'Updates dom attribute value with the current plugin > attribute value', to make it clear that we need to call this *after* setting the > attribute value to plugin. Done. https://codereview.chromium.org/11418261/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin_bindings.cc:744: return false; On 2012/11/30 22:11:36, lazyboy wrote: > nit: is break; more readable? not sure. I'm indifferent. I just changed it to break;
-sadrul, +abarth Hi Adam, Could you please take a look at this? The net effect of this change is we can observe changes to properties as changes to the DOM attribute on the <object>. These can then be propagated to the <webview> Thoughts? Any potential for side-effects here? Fady
I'm not familiar enough with this code to understand what this patch is doing. In general, if you want to observe DOM changes, WebKitMutationObserver is a good tool.
Fady and I talked a bit about this patch in chat. What he's trying to do here is implement the "reflect" semantics that we usually have in the web platform where JavaScript properties reflect the value of underlying DOM attributes. Normally the way we implement this is that the JavaScript properties are just convenient short hands for getting and setting the underlying DOM attribute, which holds the canonical copy of the data. We then observe changes to the DOM attribute to implement effects, like kicking off network loads. It looks like here we have two copies of the data (if not three): one in the DOM and one in member variables in C++. Ideally, we'd want to reduce the number of copies of the data so that there isn't any risk of the different copies getting out of sync. Normally the way we handle that is by having the only copy of the data reside in DOM attributes (because the DOM has a rich API for getting and setting DOM attributes). After this patch, we should consider how we can reduce the number of copies of this data, ideally down to one canonical copy (likely in DOM attributes).
LGTM, with a style nit, and another optional nit. https://codereview.chromium.org/11418261/diff/5003/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/11418261/diff/5003/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:264: const std::string& attribute_name, const std::string& attribute_value) { each param in own line https://codereview.chromium.org/11418261/diff/5003/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin_bindings.cc (right): https://codereview.chromium.org/11418261/diff/5003/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin_bindings.cc:406: virtual void UpdateDOMAttribute(BrowserPluginBindings* bindings) {} This could be a non-virtual function, that does: bindings->instance()->UpdateDOMAttribute(name(), GetDOMAttributeValue(bindings->instance())); where GetDOMAttributeValue is a virtual function, and each override simply returns the appropriate value that should be set (this reduces only a very little code duplication in all the overrides though)
Updated. PTAL. I now also update the <webview>'s DOM attributes in response to a change to the BrowserPlugin's DOM attribute changes. I avoid infinite loops by checking if the attribute has changed, and only mutating if there is a change. https://codereview.chromium.org/11418261/diff/5003/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/11418261/diff/5003/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:264: const std::string& attribute_name, const std::string& attribute_value) { On 2012/12/03 21:58:44, sadrul wrote: > each param in own line Done. https://codereview.chromium.org/11418261/diff/5003/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin_bindings.cc (right): https://codereview.chromium.org/11418261/diff/5003/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin_bindings.cc:406: virtual void UpdateDOMAttribute(BrowserPluginBindings* bindings) {} On 2012/12/03 21:58:44, sadrul wrote: > This could be a non-virtual function, that does: > > bindings->instance()->UpdateDOMAttribute(name(), > GetDOMAttributeValue(bindings->instance())); > > where GetDOMAttributeValue is a virtual function, and each override simply > returns the appropriate value that should be set (this reduces only a very > little code duplication in all the overrides though) I like this! Doing so.
still LGTM! https://codereview.chromium.org/11418261/diff/12003/chrome/renderer/resources... File chrome/renderer/resources/extensions/web_view.js (right): https://codereview.chromium.org/11418261/diff/12003/chrome/renderer/resources... chrome/renderer/resources/extensions/web_view.js:145: console.log('observed object mutation'); remove this? https://codereview.chromium.org/11418261/diff/12003/content/renderer/browser_... File content/renderer/browser_plugin/browser_plugin_bindings.cc (right): https://codereview.chromium.org/11418261/diff/12003/content/renderer/browser_... content/renderer/browser_plugin/browser_plugin_bindings.cc:406: return std::string(); Like SetProperty/GetProperty, keep this pure here.
Works. CQ'ing. https://codereview.chromium.org/11418261/diff/12003/chrome/renderer/resources... File chrome/renderer/resources/extensions/web_view.js (right): https://codereview.chromium.org/11418261/diff/12003/chrome/renderer/resources... chrome/renderer/resources/extensions/web_view.js:145: console.log('observed object mutation'); On 2012/12/04 01:28:46, sadrul wrote: > remove this? Done. https://codereview.chromium.org/11418261/diff/12003/content/renderer/browser_... File content/renderer/browser_plugin/browser_plugin_bindings.cc (right): https://codereview.chromium.org/11418261/diff/12003/content/renderer/browser_... content/renderer/browser_plugin/browser_plugin_bindings.cc:406: return std::string(); On 2012/12/04 01:28:46, sadrul wrote: > Like SetProperty/GetProperty, keep this pure here. Done. I agree, making this pure virtual is better so that no one forgets to add this when adding a new property binding.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11418261/4004
Err, by "works", I mean I fixed nits. :P Sorry for the momentary absent-mindedness.
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11418261/4004 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
