Move DocumentXMLTreeViewer off of private script

Move DocumentXMLTreeViewer off of private script

- Execute DocumentXMLTreeViewer.js in an isolated world
- Remove all private script hooks
- Delete idl file

BUG=672500
Committed: https://crrev.com/e2ebe3280b99674d33fc8f3c4c006080168afa51
Cr-Commit-Position: refs/heads/master@{#438224}

[adithyas, 2016-12-09 18:24:23]

The CQ bit was checked by adithyas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 18:24:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adithyas, 2016-12-09 21:09:44]

adithyas@chromium.org changed reviewers:
+ haraken@chromium.org, jbroman@chromium.org

[jbroman, 2016-12-10 20:31:55]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 20:32:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2016-12-10 20:35:31]

lgtm

Seems straightforward enough to me.

https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp (right):

https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp:23: String
scriptString(DocumentXMLTreeViewerJs.data(),
Nit: this assumes with no check that the data will be Latin-1 encoded. We
usually use UTF-8, so that essentially means these must be ASCII. They are, but
you could use loadResourceAsASCIIString from PlatformResourceLoader.h, which
checks that and does the conversion.

https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.h (right):

https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.h:18: : public
GarbageCollected<DocumentXMLTreeViewer> {
No need to make this object garbage-collected, since you only ever use it on the
stack (it can be marked STACK_ALLOCATED instead; see wtf/Allocator.h).

But further, since this object doesn't really maintain any state, I think this
could just be a function in the blink namespace with no class at all.

void transformDocumentToXMLTreeView(Document*);

[commit-bot: I haz the power, 2016-12-10 22:44:26]

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

[commit-bot: I haz the power, 2016-12-10 22:44:27]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-12-12 01:09:57]

LGTM

https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp (right):

https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp:33:
WorldIdConstants::DocumentXMLTreeViewerWorldId, sources, 0, nullptr);

Does this need to be executed in an isolated world? I guess we can just use the
main world.

Would you check what world we had been using two years ago before we converted
XMLTreeViewer to Blink-in-JS?

[adithyas, 2016-12-12 16:13:50]

https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp (right):

https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp:23: String
scriptString(DocumentXMLTreeViewerJs.data(),
On 2016/12/10 at 20:35:31, jbroman wrote:
> Nit: this assumes with no check that the data will be Latin-1 encoded. We
usually use UTF-8, so that essentially means these must be ASCII. They are, but
you could use loadResourceAsASCIIString from PlatformResourceLoader.h, which
checks that and does the conversion.

Done.

https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp:33:
WorldIdConstants::DocumentXMLTreeViewerWorldId, sources, 0, nullptr);
On 2016/12/12 at 01:09:56, haraken wrote:
> Does this need to be executed in an isolated world? I guess we can just use
the main world.
> 
> Would you check what world we had been using two years ago before we converted
XMLTreeViewer to Blink-in-JS?

We used to use the main world to execute the script. jbroman@ recommended using
an isolated world to prevent other frames from being able to access the tree
viewer's objects, which I think makes sense.

https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.h (right):

https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.h:18: : public
GarbageCollected<DocumentXMLTreeViewer> {
On 2016/12/10 at 20:35:31, jbroman wrote:
> No need to make this object garbage-collected, since you only ever use it on
the stack (it can be marked STACK_ALLOCATED instead; see wtf/Allocator.h).
> 
> But further, since this object doesn't really maintain any state, I think this
could just be a function in the blink namespace with no class at all.
> 
> void transformDocumentToXMLTreeView(Document*);

Yup, that makes sense. I've changed it to a function.

[haraken, 2016-12-12 16:15:56]

On 2016/12/12 16:13:50, adithyas wrote:
>
https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp (right):
> 
>
https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp:23: String
> scriptString(DocumentXMLTreeViewerJs.data(),
> On 2016/12/10 at 20:35:31, jbroman wrote:
> > Nit: this assumes with no check that the data will be Latin-1 encoded. We
> usually use UTF-8, so that essentially means these must be ASCII. They are,
but
> you could use loadResourceAsASCIIString from PlatformResourceLoader.h, which
> checks that and does the conversion.
> 
> Done.
> 
>
https://codereview.chromium.org/2562093002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp:33:
> WorldIdConstants::DocumentXMLTreeViewerWorldId, sources, 0, nullptr);
> On 2016/12/12 at 01:09:56, haraken wrote:
> > Does this need to be executed in an isolated world? I guess we can just use
> the main world.
> > 
> > Would you check what world we had been using two years ago before we
converted
> XMLTreeViewer to Blink-in-JS?
> 
> We used to use the main world to execute the script. jbroman@ recommended
using
> an isolated world to prevent other frames from being able to access the tree
> viewer's objects, which I think makes sense.

+1.

[adithyas, 2016-12-12 16:32:59]

adithyas@chromium.org changed reviewers:
+ esprehn@chromium.org

[adithyas, 2016-12-12 16:32:59]

+esprehn@ for content/child review

[esprehn, 2016-12-12 22:21:57]

You need a null check in there I think.

lgtm w/ that fixed.

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp (right):

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp:28:
document->getElementById("xml-viewer-style")->setTextContent(cssString);
I'm pretty sure you can actually get into a situation with a window.open()'ed
script where a mutation event would then remove this style tag and this would
crash.

You want to do:

Element* element = document->getElementById("xml-viewer-style");
if (element)
  element->setTextContent(...);

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.h (right):

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.h:12: void
transformDocumentToXMLTreeView(Document*);
Document&

[jbroman, 2016-12-13 15:41:05]

two trivial nits, since you're going to have to update this for esprehn's
comments anyhow

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp (right):

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp:14: #include
"public/platform/Platform.h"
last minute nit: this include is now unused

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp:24: v8::HandleScope
handleScope(v8::Isolate::GetCurrent());
super-nit: V8PerIsolateData::mainThreadIsolate() is probably better (doesn't
need thread-local storage, and Isolate::GetCurrent is discouraged anyhow).

[adithyas, 2016-12-13 16:18:17]

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp (right):

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp:14: #include
"public/platform/Platform.h"
On 2016/12/13 at 15:41:05, jbroman wrote:
> last minute nit: this include is now unused

Removed.

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp:24: v8::HandleScope
handleScope(v8::Isolate::GetCurrent());
On 2016/12/13 at 15:41:05, jbroman wrote:
> super-nit: V8PerIsolateData::mainThreadIsolate() is probably better (doesn't
need thread-local storage, and Isolate::GetCurrent is discouraged anyhow).

Changed to use V8PerIsolateData::mainThreadIsolate() now

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp:28:
document->getElementById("xml-viewer-style")->setTextContent(cssString);
On 2016/12/12 at 22:21:57, esprehn wrote:
> I'm pretty sure you can actually get into a situation with a window.open()'ed
script where a mutation event would then remove this style tag and this would
crash.
> 
> You want to do:
> 
> Element* element = document->getElementById("xml-viewer-style");
> if (element)
>   element->setTextContent(...);

Fixed

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.h (right):

https://codereview.chromium.org/2562093002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.h:12: void
transformDocumentToXMLTreeView(Document*);
On 2016/12/12 at 22:21:57, esprehn wrote:
> Document&

Done.

[adithyas, 2016-12-13 16:20:10]

The CQ bit was checked by adithyas@chromium.org

[adithyas, 2016-12-13 16:20:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jbroman@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2562093002/#ps120001
(title: "Use V8PerIsolateData::mainThreadIsolate()")

[commit-bot: I haz the power, 2016-12-13 16:20:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 18:11:07]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481646010581190,
"parent_rev": "969a131595a580a6f263e023bbd0a752acf59d5c", "commit_rev":
"82cc515fe6c6347129812b0ae689c36ae455cd23"}

[commit-bot: I haz the power, 2016-12-13 18:11:34]

Description was changed from

==========
Move DocumentXMLTreeViewer off of private script

- Execute DocumentXMLTreeViewer.js in an isolated world
- Remove all private script hooks
- Delete idl file

BUG=672500
==========

to

==========
Move DocumentXMLTreeViewer off of private script

- Execute DocumentXMLTreeViewer.js in an isolated world
- Remove all private script hooks
- Delete idl file

BUG=672500

Review-Url: https://codereview.chromium.org/2562093002
==========

[commit-bot: I haz the power, 2016-12-13 18:11:35]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-13 18:15:07]

Description was changed from

==========
Move DocumentXMLTreeViewer off of private script

- Execute DocumentXMLTreeViewer.js in an isolated world
- Remove all private script hooks
- Delete idl file

BUG=672500

Review-Url: https://codereview.chromium.org/2562093002
==========

to

==========
Move DocumentXMLTreeViewer off of private script

- Execute DocumentXMLTreeViewer.js in an isolated world
- Remove all private script hooks
- Delete idl file

BUG=672500

Committed: https://crrev.com/e2ebe3280b99674d33fc8f3c4c006080168afa51
Cr-Commit-Position: refs/heads/master@{#438224}
==========

[commit-bot: I haz the power, 2016-12-13 18:15:08]

Patchset 7 (id:??) landed as
https://crrev.com/e2ebe3280b99674d33fc8f3c4c006080168afa51
Cr-Commit-Position: refs/heads/master@{#438224}