Refactored extensionOptions to use the new guestViewAttributes module.

Refactored extensionOptions to use the new guestViewAttributes module.

BUG=466215
Committed: https://crrev.com/99d84da562b39b543ab695f3707399b8bc2f2405
Cr-Commit-Position: refs/heads/master@{#322044}

[paulmeyer, 2015-03-20 18:22:43]

paulmeyer@chromium.org changed reviewers:
+ fsamuel@chromium.org, kalman@chromium.org

[paulmeyer, 2015-03-20 18:22:44]

+fsamuel@ for guest_view/

+kalman@ for dispatcher.cc and extensions_renderer_resources.grd

[not at google - send to devlin, 2015-03-20 19:15:23]

dispatcher.cc / grd lgtm.

[Fady Samuel, 2015-03-20 19:53:58]

https://codereview.chromium.org/1026703004/diff/1/extensions/renderer/resourc...
File
extensions/renderer/resources/guest_view/extension_options/extension_options.js
(right):

https://codereview.chromium.org/1026703004/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/guest_view/extension_options/extension_options.js:27:
'extensionId': this.attributes[
Can we simply iterate over the attributes array here? You probably need to make
some changes to ExtensionOptionsGuest too.

https://codereview.chromium.org/1026703004/diff/1/extensions/renderer/resourc...
File
extensions/renderer/resources/guest_view/extension_options/extension_options_attributes.js
(right):

https://codereview.chromium.org/1026703004/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/guest_view/extension_options/extension_options_attributes.js:25:
// We treat null attribute (attribute removed) and the empty string as
We will never hit this case I think. I believe handleMutation will only get
called if oldValue != newValue.

[Fady Samuel, 2015-03-20 20:11:12]

https://codereview.chromium.org/1026703004/diff/1/extensions/renderer/resourc...
File
extensions/renderer/resources/guest_view/extension_options/extension_options.js
(right):

https://codereview.chromium.org/1026703004/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/guest_view/extension_options/extension_options.js:33:
if (!this.elementAttached) {
Can we move some code to the ExtensionAttribute?

The check for elementAttached can be moved.

[paulmeyer, 2015-03-24 17:31:42]

ptal.

https://codereview.chromium.org/1026703004/diff/1/extensions/renderer/resourc...
File
extensions/renderer/resources/guest_view/extension_options/extension_options.js
(right):

https://codereview.chromium.org/1026703004/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/guest_view/extension_options/extension_options.js:27:
'extensionId': this.attributes[
On 2015/03/20 19:53:57, Fady Samuel wrote:
> Can we simply iterate over the attributes array here? You probably need to
make
> some changes to ExtensionOptionsGuest too.

Done.

https://codereview.chromium.org/1026703004/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/guest_view/extension_options/extension_options.js:33:
if (!this.elementAttached) {
On 2015/03/20 20:11:12, Fady Samuel wrote:
> Can we move some code to the ExtensionAttribute?
> 
> The check for elementAttached can be moved. 

You're right about the check... but I think the rest of createGuest() still
makes sense to stay here since it is more related to the container than the
attribute (I think).

https://codereview.chromium.org/1026703004/diff/1/extensions/renderer/resourc...
File
extensions/renderer/resources/guest_view/extension_options/extension_options_attributes.js
(right):

https://codereview.chromium.org/1026703004/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/guest_view/extension_options/extension_options_attributes.js:25:
// We treat null attribute (attribute removed) and the empty string as
On 2015/03/20 19:53:57, Fady Samuel wrote:
> We will never hit this case I think. I believe handleMutation will only get
> called if oldValue != newValue.

This case is hit if, say, the extension attribute is set to empty string and
then it is removed entirely.

[Fady Samuel, 2015-03-24 17:37:29]

https://codereview.chromium.org/1026703004/diff/20001/extensions/renderer/res...
File
extensions/renderer/resources/guest_view/extension_options/extension_options_attributes.js
(right):

https://codereview.chromium.org/1026703004/diff/20001/extensions/renderer/res...
extensions/renderer/resources/guest_view/extension_options/extension_options_attributes.js:28:
newValue = newValue || '';
This behavior is really confusing. This should behave a bit like the src
attribute of <webview>.

If we remove the extension attribute, we should add it back. Otherwise, if we
change it we should call createGuest.

[paulmeyer, 2015-03-24 18:13:17]

Patchset #3 (id:40001) has been deleted

[paulmeyer, 2015-03-24 18:13:40]

ptal

https://codereview.chromium.org/1026703004/diff/20001/extensions/renderer/res...
File
extensions/renderer/resources/guest_view/extension_options/extension_options_attributes.js
(right):

https://codereview.chromium.org/1026703004/diff/20001/extensions/renderer/res...
extensions/renderer/resources/guest_view/extension_options/extension_options_attributes.js:28:
newValue = newValue || '';
On 2015/03/24 17:37:29, Fady Samuel wrote:
> This behavior is really confusing. This should behave a bit like the src
> attribute of <webview>.
> 
> If we remove the extension attribute, we should add it back. Otherwise, if we
> change it we should call createGuest.

Done.

[Fady Samuel, 2015-03-24 18:20:17]

lgtm

[paulmeyer, 2015-03-24 18:20:45]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2015-03-24 18:20:46]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1026703004/#ps60001
(title: "Addressed comment.")

[commit-bot: I haz the power, 2015-03-24 18:21:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1026703004/60001

[commit-bot: I haz the power, 2015-03-24 19:21:53]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-03-24 19:22:39]

Patchset 3 (id:??) landed as
https://crrev.com/99d84da562b39b543ab695f3707399b8bc2f2405
Cr-Commit-Position: refs/heads/master@{#322044}