Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
479753003:
Transform XML tree viewer to blink-in-js implementation. (Closed)
|
Created:
6 years, 3 months ago by vivekg_samsung Modified:
6 years, 3 months ago Reviewers:
hajimehoshi, vsevik, yurys, eseidel, vivekg, pfeldman, haraken, abarth-chromium, yosin_UTC9, PhistucK CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionTransform XML tree viewer to blink-in-js implementation.
This CL moves the XML tree viewer to use blink-in-js implementation. As part of this CL
the XMLTreeViewer.idl exposes transformDocumentToTreeView which is used by XMLDocumentParser
to transform into tree view.
This CL should follow up with the removal of Source/core/xml/XMLViewer.[css/js] and the subsequent
changes in public/blink_resources.grd.
BUG=341031
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181328
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 5
Patch Set 4 : #
Total comments: 1
Patch Set 5 : Fixed declaration order in core_generated.gyp #Patch Set 6 : Patch for landing! #
Total comments: 4
Patch Set 7 : #
Messages
Total messages: 51 (3 generated)
vivekg@chromium.org changed reviewers: + haraken@chromium.org, vivekg@chromium.org
PTAL, thank you!
haraken@chromium.org changed reviewers: + abarth@chromium.org, hajimehoshi@chromium.org
You'll need to add XMLViewer.idl and XMLViewer.js ?
eseidel@chromium.org changed reviewers: + eseidel@chromium.org
https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewer.h File Source/core/xml/XMLTreeViewer.h (right): https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewe... Source/core/xml/XMLTreeViewer.h:39: class XMLTreeViewer FINAL : public GarbageCollectedFinalized<XMLTreeViewer>, public ScriptWrappable { Does this need Finialized? Or just plain GarbageCollected? https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewe... Source/core/xml/XMLTreeViewer.h:42: explicit XMLTreeViewer() { ScriptWrappable::init(this); } Why the explicit? https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewe... Source/core/xml/XMLTreeViewer.h:43: void trace(Visitor*) { } I'm surprised this is needed?
On 2014/08/26 05:38:42, haraken wrote: > You'll need to add XMLViewer.idl and XMLViewer.js ? Yeah missed to add them in the CL. Added now, thanks!
https://codereview.chromium.org/479753003/diff/20001/Source/core/xml/XMLTreeV... File Source/core/xml/XMLTreeViewer.idl (right): https://codereview.chromium.org/479753003/diff/20001/Source/core/xml/XMLTreeV... Source/core/xml/XMLTreeViewer.idl:7: [ImplementedInPrivateScript, OnlyExposedToPrivateScript] void transformDocumentToTreeView(); What's the point of moving this to Blink-in-JS if we have to add backdoors to implement it?
https://codereview.chromium.org/479753003/diff/20001/Source/core/xml/XMLTreeV... File Source/core/xml/XMLTreeViewer.h (right): https://codereview.chromium.org/479753003/diff/20001/Source/core/xml/XMLTreeV... Source/core/xml/XMLTreeViewer.h:39: class XMLTreeViewer FINAL : public GarbageCollectedFinalized<XMLTreeViewer>, public ScriptWrappable { Can't we generate this class automatically? As can be seen, there isn't any work done by this class specifically.
On 2014/08/26 at 05:39:28, eseidel wrote: > https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewer.h > File Source/core/xml/XMLTreeViewer.h (right): > > https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewe... > Source/core/xml/XMLTreeViewer.h:39: class XMLTreeViewer FINAL : public GarbageCollectedFinalized<XMLTreeViewer>, public ScriptWrappable { > Does this need Finialized? Or just plain GarbageCollected? > > https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewe... > Source/core/xml/XMLTreeViewer.h:42: explicit XMLTreeViewer() { ScriptWrappable::init(this); } > Why the explicit? > > https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewe... > Source/core/xml/XMLTreeViewer.h:43: void trace(Visitor*) { } > I'm surprised this is needed? Yeah, I think the whole HolderImpl can be generated by the binding scripts. In that case, there is no need of this dummy class here altogether.
https://codereview.chromium.org/479753003/diff/20001/Source/core/xml/XMLTreeV... File Source/core/xml/XMLTreeViewer.idl (right): https://codereview.chromium.org/479753003/diff/20001/Source/core/xml/XMLTreeV... Source/core/xml/XMLTreeViewer.idl:7: [ImplementedInPrivateScript, OnlyExposedToPrivateScript] void transformDocumentToTreeView(); On 2014/08/26 at 05:40:54, eseidel wrote: > What's the point of moving this to Blink-in-JS if we have to add backdoors to implement it? Sorry @eseidel, I could not get what meant by saying 'add backdoors to implement it?'.
Sorry, I misunderstood how that idl function worked. I see now that this is entirely impelemnted in javascxript.
https://codereview.chromium.org/479753003/diff/20001/Source/core/core_generat... File Source/core/core_generated.gyp (right): https://codereview.chromium.org/479753003/diff/20001/Source/core/core_generat... Source/core/core_generated.gyp:154: 'xml/XMLTreeViewer.js', do these need to be root relative? I'm surprised the marquee one uses ../core
https://codereview.chromium.org/479753003/diff/20001/Source/core/xml/XMLTreeV... File Source/core/xml/XMLTreeViewer.h (right): https://codereview.chromium.org/479753003/diff/20001/Source/core/xml/XMLTreeV... Source/core/xml/XMLTreeViewer.h:39: class XMLTreeViewer FINAL : public GarbageCollectedFinalized<XMLTreeViewer>, public ScriptWrappable { On 2014/08/26 05:41:48, vivekg_ wrote: > Can't we generate this class automatically? As can be seen, there isn't any work > done by this class specifically. This class shouldn't be necessary at all. We need to improve the IDL compiler so that it doesn't generate code that requires XMLTreeViewer. https://codereview.chromium.org/479753003/diff/20001/Source/core/xml/XMLTreeV... File Source/core/xml/XMLTreeViewer.idl (right): https://codereview.chromium.org/479753003/diff/20001/Source/core/xml/XMLTreeV... Source/core/xml/XMLTreeViewer.idl:7: [ImplementedInPrivateScript, OnlyExposedToPrivateScript] void transformDocumentToTreeView(); On 2014/08/26 05:40:54, eseidel wrote: > What's the point of moving this to Blink-in-JS if we have to add backdoors to > implement it? I'm considering a way to avoid the backdoor (but at least this CL is not making our current situation worse; i.e., the current XMLTreeViewer is already implemented in JS and working by manually getting invoked from C++). Either way I think we should move this method to Document.idl (or to DocumentXMLTreeViewer.idl, which is a partial interface of Document.idl). That way we no longer need an interface for XMLTreeViewer. Then you can completely remove the XMLTreeViewer class.
On 2014/08/26 05:54:17, eseidel wrote: > Sorry, I misunderstood how that idl function worked. I see now that this is > entirely impelemnted in javascxript. Yes. XMLTreeViewer is already implemented in JS (before this CL) and the JS is working by manually getting invoked from C++. In other words, the current XMLTreeViewer is already working using the backdoor. In that sense, this CL is not making the current situation any worse. However, it would be great if we could get rid of the backdoor.
On 2014/08/26 05:49:19, vivekg_ wrote: > On 2014/08/26 at 05:39:28, eseidel wrote: > > > https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewer.h > > File Source/core/xml/XMLTreeViewer.h (right): > > > > > https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewe... > > Source/core/xml/XMLTreeViewer.h:39: class XMLTreeViewer FINAL : public > GarbageCollectedFinalized<XMLTreeViewer>, public ScriptWrappable { > > Does this need Finialized? Or just plain GarbageCollected? > > > > > https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewe... > > Source/core/xml/XMLTreeViewer.h:42: explicit XMLTreeViewer() { > ScriptWrappable::init(this); } > > Why the explicit? > > > > > https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewe... > > Source/core/xml/XMLTreeViewer.h:43: void trace(Visitor*) { } > > I'm surprised this is needed? > > Yeah, I think the whole HolderImpl can be generated by the binding scripts. In > that case, there is no need of this dummy class here altogether. In my ideal world, an XMLTreeViewer object is not needed at all :) Given that we don't need to keep any state on the XMLTreeViewer, no instance will be needed. As commented above, I think you just need to move the transform method to a partial interface of Document.idl. Then you no longer need XMLTreeViewer.
On 2014/08/26 at 06:03:37, haraken wrote: > On 2014/08/26 05:49:19, vivekg_ wrote: > > On 2014/08/26 at 05:39:28, eseidel wrote: > > > > > https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewer.h > > > File Source/core/xml/XMLTreeViewer.h (right): > > > > > > > > https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewe... > > > Source/core/xml/XMLTreeViewer.h:39: class XMLTreeViewer FINAL : public > > GarbageCollectedFinalized<XMLTreeViewer>, public ScriptWrappable { > > > Does this need Finialized? Or just plain GarbageCollected? > > > > > > > > https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewe... > > > Source/core/xml/XMLTreeViewer.h:42: explicit XMLTreeViewer() { > > ScriptWrappable::init(this); } > > > Why the explicit? > > > > > > > > https://codereview.chromium.org/479753003/diff/1/Source/core/xml/XMLTreeViewe... > > > Source/core/xml/XMLTreeViewer.h:43: void trace(Visitor*) { } > > > I'm surprised this is needed? > > > > Yeah, I think the whole HolderImpl can be generated by the binding scripts. In > > that case, there is no need of this dummy class here altogether. > > In my ideal world, an XMLTreeViewer object is not needed at all :) Given that we don't need to keep any state on the XMLTreeViewer, no instance will be needed. > > As commented above, I think you just need to move the transform method to a partial interface of Document.idl. Then you no longer need XMLTreeViewer. Done
Mostly looks good. Just to confirm: XMLTreeViewer.js is basically just a copy of the old XMLTreeViewer.js, right? I haven't reviewed it. https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... File Source/core/xml/XMLTreeViewer.idl (right): https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... Source/core/xml/XMLTreeViewer.idl:4: partial interface Document { Add one empty line after the copyright. https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... Source/core/xml/XMLTreeViewer.idl:4: partial interface Document { Rename this file to DocumentXMLTreeViewer.idl. We conventionally prefix an IDL file of a partial interface with a base interface name. https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... File Source/core/xml/XMLTreeViewer.js (right): https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... Source/core/xml/XMLTreeViewer.js:1: "use strict"; Add a copyright. https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... Source/core/xml/XMLTreeViewer.js:4: var styleSheet = [ We should have a way to write a CSS in a separate file. Can you add a FIXME?
https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... File Source/core/xml/XMLTreeViewer.idl (right): https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... Source/core/xml/XMLTreeViewer.idl:5: [ImplementedInPrivateScript, OnlyExposedToPrivateScript] void transformDocumentToTreeView(); abarth@: I feel OK with this (reverse-)backdoor, but what do you think? XMLTreeViewer is not a web-exposed feature and thus we'll anyway need a way to kick off the viewer script from C++. In other words, we'll anyway need one reverse-backdoor ("reverse-" means that this is a backdoor for C++ to kick off JavaScript).
On 2014/08/26 at 08:05:08, haraken wrote: > Mostly looks good. > > Just to confirm: XMLTreeViewer.js is basically just a copy of the old XMLTreeViewer.js, right? I haven't reviewed it. > Yes, its the same file. Just moved it inside the installClass function definition. > https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... > File Source/core/xml/XMLTreeViewer.idl (right): > > https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... > Source/core/xml/XMLTreeViewer.idl:4: partial interface Document { > > Add one empty line after the copyright. > Done. > https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... > Source/core/xml/XMLTreeViewer.idl:4: partial interface Document { > > Rename this file to DocumentXMLTreeViewer.idl. We conventionally prefix an IDL file of a partial interface with a base interface name. > Done. > https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... > File Source/core/xml/XMLTreeViewer.js (right): > > https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... > Source/core/xml/XMLTreeViewer.js:1: "use strict"; > > Add a copyright. > Done. > https://codereview.chromium.org/479753003/diff/40001/Source/core/xml/XMLTreeV... > Source/core/xml/XMLTreeViewer.js:4: var styleSheet = [ > > We should have a way to write a CSS in a separate file. Can you add a FIXME? Done.
LGTM, but let's wait for abarth@. https://codereview.chromium.org/479753003/diff/60001/Source/core/xml/Document... File Source/core/xml/DocumentXMLTreeViewer.h (right): https://codereview.chromium.org/479753003/diff/60001/Source/core/xml/Document... Source/core/xml/DocumentXMLTreeViewer.h:11: // DocumentXMLTreeViewer.h. We should improve the IDL compiler and remove this file. I'll fix this in a follow-up CL.
On 2014/08/26 at 08:56:19, haraken wrote: > LGTM, but let's wait for abarth@. > Sure, thank you!
vivekg@: Looks like xmlviewer/extensions-api.html is failing. abarth@: Would you take a look at the private script API in DocumentXMLTreeViewer.idl? I guess the backdoor is fine in this case as commented in #20.
On 2014/08/27 at 01:09:16, haraken wrote: > abarth@: Would you take a look at the private script API in DocumentXMLTreeViewer.idl? I guess the backdoor is fine in this case as commented in #20. Yes, that looks fine. Thanks!
I have changed the code to bubble the load event further so that the extension can listen on it. Modified the test accordingly. If we agree on deprecation of onAfterWebkitXMLViewerLoaded in lieu of 'load' event, shall we send a deprecation warning on blink-dev or chromium-dev?
vivekg@chromium.org changed reviewers: + pfeldman@chromium.org
+pfeldman for deprecation of onAfterWebkitXMLViewerLoaded
haraken@chromium.org changed reviewers: + yurys@chromium.org
Looks like pfeldman@ is OOO for a while...
yurys@, abarth@: In a nutshell, we are going to deprecate the
onAfterWebkitXMLViewerLoaded event from extension APIs, but would it be OK?
Before this CL, the onAfterWebkitXMLViewerLoaded event was used in the following
way:
// user's extension script
window.onAfterWebkitXMLViewerLoaded = function () { ... }
// XMLViewer.js in Blink
function viewerLoaded() {
if (window.onAfterWebkitXMLViewerLoaded)
window.onAfterWebkitXMLViewerLoaded();
}
Once we move XMLViewer.js to Blink-in-JS, we cannot support this API because the
window object of user's extension script is different from the window object of
the private script. Thus we are going to deprecate the
onAfterWebkitXMLViewerLoaded event and replace it with a standard window.onload
event (i.e., the user's extension script can just listen a normal window.onload
event).
As far as I do Google search, the onAfterWebkitXMLViewerLoaded event is used
only in our layout test (i.e., extension-api.html). So I think it's OK to
deprecate.
yurys@chromium.org changed reviewers: + vsevik@chromium.org
+vsevik who worked with this code.
I know two extensions for prettifying XML: XV - XML Viewer (https://chrome.google.com/webstore/detail/xv-%E2%80%94-xml-viewer/eeocglpgjdp...) and XML Tree (https://chrome.google.com/webstore/detail/xml-tree/gbammbheopgpmaagmckhpjbfgd...). Alan Stroop, the author of the second one was the one who originally asked us to add this "API". I took look on both of them and it seems like they do not use this API at this moment. I think it's fine to remove it, but I would ask you to check that both extensions still work after your change. Be aware that they might require XML file to be served by HTTP and not work with local files. Other than that, I think a short message to blink-dev should be enough.
Also, why don't you remove XMLViewer.js/css files in the same patch?
On 2014/08/28 11:58:18, vsevik wrote: > I know two extensions for prettifying XML: XV - XML Viewer > (https://chrome.google.com/webstore/detail/xv-%E2%80%94-xml-viewer/eeocglpgjdp...) > and XML Tree > (https://chrome.google.com/webstore/detail/xml-tree/gbammbheopgpmaagmckhpjbfgd...). > > Alan Stroop, the author of the second one was the one who originally asked us to > add this "API". > I took look on both of them and it seems like they do not use this API at this > moment. > > I think it's fine to remove it, but I would ask you to check that both > extensions still work after your change. Be aware that they might require XML > file to be served by HTTP and not work with local files. > > Other than that, I think a short message to blink-dev should be enough. Thanks vsevik! That sounds reasonable to me.
On 2014/08/28 at 11:58:56, vsevik wrote: > Also, why don't you remove XMLViewer.js/css files in the same patch? This has a dependency on chromium side changes. This is a 3-way patch which I would be handling in the subsequent cleanup once this patch lands.
On 2014/08/28 at 11:58:18, vsevik wrote: > I know two extensions for prettifying XML: XV - XML Viewer (https://chrome.google.com/webstore/detail/xv-%E2%80%94-xml-viewer/eeocglpgjdp...) and XML Tree (https://chrome.google.com/webstore/detail/xml-tree/gbammbheopgpmaagmckhpjbfgd...). > > Alan Stroop, the author of the second one was the one who originally asked us to add this "API". > I took look on both of them and it seems like they do not use this API at this moment. > > I think it's fine to remove it, but I would ask you to check that both extensions still work after your change. Be aware that they might require XML file to be served by HTTP and not work with local files. Yes these are not using the mentioned API and work well with the changes. > > Other than that, I think a short message to blink-dev should be enough. Sure, I would send it out. Thank you.
On 2014/09/02 07:00:14, vivekg_ wrote: > On 2014/08/28 at 11:58:18, vsevik wrote: > > I know two extensions for prettifying XML: XV - XML Viewer > (https://chrome.google.com/webstore/detail/xv-%E2%80%94-xml-viewer/eeocglpgjdp...) > and XML Tree > (https://chrome.google.com/webstore/detail/xml-tree/gbammbheopgpmaagmckhpjbfgd...). > > > > Alan Stroop, the author of the second one was the one who originally asked us > to add this "API". > > I took look on both of them and it seems like they do not use this API at this > moment. > > > > I think it's fine to remove it, but I would ask you to check that both > extensions still work after your change. Be aware that they might require XML > file to be served by HTTP and not work with local files. > > Yes these are not using the mentioned API and work well with the changes. > > > > > Other than that, I think a short message to blink-dev should be enough. > > Sure, I would send it out. Thank you. Thanks, we're getting there :)
On 2014/09/02 at 07:27:31, haraken wrote: > On 2014/09/02 07:00:14, vivekg_ wrote: > > On 2014/08/28 at 11:58:18, vsevik wrote: > > > I know two extensions for prettifying XML: XV - XML Viewer > > (https://chrome.google.com/webstore/detail/xv-%E2%80%94-xml-viewer/eeocglpgjdp...) > > and XML Tree > > (https://chrome.google.com/webstore/detail/xml-tree/gbammbheopgpmaagmckhpjbfgd...). > > > > > > Alan Stroop, the author of the second one was the one who originally asked us > > to add this "API". > > > I took look on both of them and it seems like they do not use this API at this > > moment. > > > > > > I think it's fine to remove it, but I would ask you to check that both > > extensions still work after your change. Be aware that they might require XML > > file to be served by HTTP and not work with local files. > > > > Yes these are not using the mentioned API and work well with the changes. > > > > > > > > Other than that, I think a short message to blink-dev should be enough. > > > > Sure, I would send it out. Thank you. > > Thanks, we're getting there :) Oops, I sent it already before your message!
phistuck@gmail.com changed reviewers: + phistuck@gmail.com
A drive by review. I only highlighted a few cases, but the entire file should be scanned for unnecessary lookups. https://codereview.chromium.org/479753003/diff/100001/Source/core/xml/Documen... File Source/core/xml/DocumentXMLTreeViewer.js (right): https://codereview.chromium.org/479753003/diff/100001/Source/core/xml/Documen... Source/core/xml/DocumentXMLTreeViewer.js:121: processNode.processorsMap = {}; Perhaps cache processNode.processorsMap instead of performing this lookup every time? It is less verbose and more performant. var map = processNode.processorsMap; if (!map) { map = {}; processNode.processorsMap = map; map[Node.PROCESSING_INSTRUCTION_NODE] = processProcessingInstruction; map[Node.ELEMENT_NODE] = processElement; https://codereview.chromium.org/479753003/diff/100001/Source/core/xml/Documen... Source/core/xml/DocumentXMLTreeViewer.js:166: collapsible.expanded.start.appendChild(createTag(node, false, false)); Same here. https://codereview.chromium.org/479753003/diff/100001/Source/core/xml/Documen... Source/core/xml/DocumentXMLTreeViewer.js:171: collapsible.collapsed.content.appendChild(createTag(node, false, false)); Same here.
yosin@chromium.org changed reviewers: + yosin@chromium.org
https://codereview.chromium.org/479753003/diff/100001/Source/core/xml/Documen... File Source/core/xml/DocumentXMLTreeViewer.idl (right): https://codereview.chromium.org/479753003/diff/100001/Source/core/xml/Documen... Source/core/xml/DocumentXMLTreeViewer.idl:5: partial interface Document { nit: Could you add extended attribute [NoImplHeader]? Once, interface has this attribute, we don't need to have .h file.
On 2014/09/02 at 08:01:55, yosin wrote: > https://codereview.chromium.org/479753003/diff/100001/Source/core/xml/Documen... > File Source/core/xml/DocumentXMLTreeViewer.idl (right): > > https://codereview.chromium.org/479753003/diff/100001/Source/core/xml/Documen... > Source/core/xml/DocumentXMLTreeViewer.idl:5: partial interface Document { > nit: Could you add extended attribute [NoImplHeader]? Once, interface has this attribute, we don't need to have .h file. Done.
On 2014/09/02 at 07:36:45, phistuck wrote: > A drive by review. > > I only highlighted a few cases, but the entire file should be scanned for unnecessary lookups. > > https://codereview.chromium.org/479753003/diff/100001/Source/core/xml/Documen... > File Source/core/xml/DocumentXMLTreeViewer.js (right): > > https://codereview.chromium.org/479753003/diff/100001/Source/core/xml/Documen... > Source/core/xml/DocumentXMLTreeViewer.js:121: processNode.processorsMap = {}; > Perhaps cache processNode.processorsMap instead of performing this lookup every time? It is less verbose and more performant. > > var map = processNode.processorsMap; > if (!map) { > map = {}; > processNode.processorsMap = map; > map[Node.PROCESSING_INSTRUCTION_NODE] = processProcessingInstruction; > map[Node.ELEMENT_NODE] = processElement; > Done. These sort of improvements can occur subseuquently as the initial plan was first to migrate the XML viewer to blink-in-js first 'as is'.
LGTM. Let's wait for a couple of days to give folks a chance to comment on the blink-dev thread.
On 2014/09/02 at 09:10:45, haraken wrote: > LGTM. > > Let's wait for a couple of days to give folks a chance to comment on the blink-dev thread. yeah sure.
On 2014/09/02 09:14:13, vivekg_ wrote: > On 2014/09/02 at 09:10:45, haraken wrote: > > LGTM. > > > > Let's wait for a couple of days to give folks a chance to comment on the > blink-dev thread. > > yeah sure. I think it's now OK to go.
The CQ bit was checked by vivekg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/479753003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 181328 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
