Remove attributes and methods which should not be part of the public API

Remove attributes and methods which should not be part of the public API

BUG=309681
Committed: https://crrev.com/8b79607ff08e34877b2a8e46c4d72a21b4841951
Cr-Commit-Position: refs/heads/master@{#292835}

[aboxhall, 2014-08-28 21:03:52]

aboxhall@chromium.org changed reviewers:
+ dmazzoni@chromium.org, dtseng@chromium.org

[David Tseng, 2014-08-28 23:30:26]

Did we want to keep toString as an undocumented API?


On Thu, Aug 28, 2014 at 2:03 PM, <aboxhall@chromium.org> wrote:

> Reviewers: David Tseng, dmazzoni,
>
> Description:
> Remove attributes and methods which should not be part of the public API
>
> BUG=309681
>
> Please review this at https://codereview.chromium.org/514363002/
>
> SVN Base: https://chromium.googlesource.com/chromium/src@master
>
> Affected files (+2, -5 lines):
>   M chrome/renderer/resources/extensions/automation/automation_node.js
>
>
> Index: chrome/renderer/resources/extensions/automation/automation_node.js
> diff --git
a/chrome/renderer/resources/extensions/automation/automation_node.js
> b/chrome/renderer/resources/extensions/automation/automation_node.js
> index 7a1854a4ce78c23bb9d7df52e170908b8405e953..
> 14648ad1f09a7d0919c222d71b5f0cbe7d30a24b 100644
> --- a/chrome/renderer/resources/extensions/automation/automation_node.js
> +++ b/chrome/renderer/resources/extensions/automation/automation_node.js
> @@ -602,16 +602,13 @@ var AutomationNode = utils.expose('AutomationNode',
>                                                  'makeVisible',
>                                                  'setSelection',
>                                                  'addEventListener',
> -                                                'removeEventListener',
> -                                                'toString'],
> +                                                'removeEventListener'],
>                                      readonly: ['isRootNode',
> -                                               'id',
>                                                 'role',
>                                                 'state',
>                                                 'location',
>                                                 'attributes',
> -                                               'root',
> -                                               'toString'] });
> +                                               'root'] });
>
>  var AutomationRootNode = utils.expose('AutomationRootNode',
>                                        AutomationRootNodeImpl,
>
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aboxhall, 2014-08-29 02:49:05]

On 2014/08/28 23:30:26, David Tseng wrote:
> Did we want to keep toString as an undocumented API?

My concern with toString is that it leaks implementation details (node IDs,
really).

I toyed with the idea of making a public version but then decided it wasn't
enough of a win to be worth it. People can use JSON.stringify if they want to
see what properties it has.

[David Tseng, 2014-08-29 16:48:25]

lgtm

I don't have a strong opinion; it may be useful for debugging, but is easily
accomplished by a client as you pointed out.

[aboxhall, 2014-08-29 17:25:01]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2014-08-29 17:25:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aboxhall@chromium.org/514363002/1

[aboxhall, 2014-08-29 17:37:40]

On 2014/08/29 16:48:25, David Tseng wrote:
> lgtm
> 
> I don't have a strong opinion; it may be useful for debugging, but is easily
> accomplished by a client as you pointed out.

Actually JSON.stringify won't work, but I still think we should remove this - as
you say, someone could easily write their own stringifier trivially.

[aboxhall, 2014-08-29 18:06:14]

The CQ bit was unchecked by aboxhall@chromium.org

[aboxhall, 2014-08-29 18:06:15]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2014-08-29 18:06:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aboxhall@chromium.org/514363002/1

[commit-bot: I haz the power, 2014-08-29 18:43:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-29 19:19:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-29 19:20:00]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aboxhall, 2014-08-29 20:51:53]

@dtseng PTAL: I had to make many more changes as a lot of things relied on .id
being available on the wrapper object.

[David Tseng, 2014-08-29 22:02:23]

https://codereview.chromium.org/514363002/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/automation/automation_node.js (left):

https://codereview.chromium.org/514363002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/automation/automation_node.js:466: var
parentId = 0;
nit: We use 0 elsewhere as a valid id. Maybe -1?

https://codereview.chromium.org/514363002/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/514363002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/automation/automation_node.js:468: var
parentImpl = privates(childNode.parent()).impl;
This should be in the if clause's body right?

https://codereview.chromium.org/514363002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/automation/automation_node.js:469: if
(childNode.parent()) parentId = parentImpl.id;
if (...)
  ...

(the one line if is pretty hard to read) and you'll need it for the previous
comment.

[David Tseng, 2014-08-29 22:12:02]

lgtm

With comments resolved.

[aboxhall, 2014-08-30 17:46:42]

https://codereview.chromium.org/514363002/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/automation/automation_node.js (left):

https://codereview.chromium.org/514363002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/automation/automation_node.js:466: var
parentId = 0;
On 2014/08/29 22:02:23, David Tseng wrote:
> nit: We use 0 elsewhere as a valid id. Maybe -1?

Done.

https://codereview.chromium.org/514363002/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/514363002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/automation/automation_node.js:468: var
parentImpl = privates(childNode.parent()).impl;
On 2014/08/29 22:02:23, David Tseng wrote:
> This should be in the if clause's body right?

Done.

https://codereview.chromium.org/514363002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/automation/automation_node.js:469: if
(childNode.parent()) parentId = parentImpl.id;
On 2014/08/29 22:02:23, David Tseng wrote:
> if (...)
>   ...
> 
> (the one line if is pretty hard to read) and you'll need it for the previous
> comment.

Done.

[aboxhall, 2014-08-30 17:46:47]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2014-08-30 17:47:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aboxhall@chromium.org/514363002/40001

[commit-bot: I haz the power, 2014-08-30 18:47:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-30 19:21:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-30 19:21:39]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aboxhall, 2014-09-01 04:28:35]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2014-09-01 04:29:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aboxhall@chromium.org/514363002/60001

[aboxhall, 2014-09-01 04:30:50]

@dtseng: Heads-up that I needed to rewrite the "Unit" test in terms of HTML IDs
rather than Automation IDs. Hopefully that's ok with you.

[commit-bot: I haz the power, 2014-09-01 05:31:37]

Committed patchset #4 (id:60001) as 5cd1acab180becdc0d837d59190972ebd38957fa

[David Tseng, 2014-09-02 15:45:41]

Yeah, seems fine. Since it was a unit test context, you could have used
privates to access the AutomationNodeImpl; not sure which would have been
more work to fix the test.


On Sun, Aug 31, 2014 at 9:30 PM, <aboxhall@chromium.org> wrote:

> @dtseng: Heads-up that I needed to rewrite the "Unit" test in terms of
> HTML IDs
> rather than Automation IDs. Hopefully that's ok with you.
>
>
> https://codereview.chromium.org/514363002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-09-10 03:15:38]

Patchset 4 (id:??) landed as
https://crrev.com/8b79607ff08e34877b2a8e46c4d72a21b4841951
Cr-Commit-Position: refs/heads/master@{#292835}