Styles pane is made work with IPC. Still not work and will be fixed later:...

Styles pane is made work with IPC. Still not work and will be fixed later:
- disabling/enabling styles (not implemented to simplify the change)
- grouping properties (needs additional investigation: it seams a style object created in such a way behaves differently)

[pfeldman, 2009-03-30 12:12:44]

http://codereview.chromium.org/57007/diff/1007/1008
File webkit/glue/devtools/dom_agent.h (right):

http://codereview.chromium.org/57007/diff/1007/1008#newcode33
Line 33: /* Requests that the element's style information are sent to the
client. */ \
nit: ... is sent to the ...

http://codereview.chromium.org/57007/diff/1007/1008#newcode34
Line 34: METHOD3(GetNodeStyles, int /* call_id */, int /* id */, bool /*
authorOnly */) \
nit: node_id, author_only.

http://codereview.chromium.org/57007/diff/1007/1008#newcode45
Line 45: METHOD2(DidGetNodeStyles, int /* call_id */, Value /* styles */) \
Please preserve method order as in direct calls.

http://codereview.chromium.org/57007/diff/1007/1009
File webkit/glue/devtools/dom_agent_impl.cc (right):

http://codereview.chromium.org/57007/diff/1007/1009#newcode429
Line 429: OwnPtr<ListValue> style_attributes(new ListValue());
Given that this is not performance-critical operation, I would prefer Dictionary
with named properties over the list. See how this is done in the net_agent.

http://codereview.chromium.org/57007/diff/1007/1009#newcode433
Line 433: if (node && (node->nodeType() == Node::ELEMENT_NODE)) {
given the size of the branch, I would suggest to replace nesting with guard
expression. i.e.
if (!node || node->nodeType() != ...) {
  DidGetNodeStyles(call_id, styles);
  return;
}

... rest of the code ...

http://codereview.chromium.org/57007/diff/1007/1009#newcode439
Line 439: DOMWindow* window = document ? document->defaultView() : NULL;
I don't think document can be null. Can it?

http://codereview.chromium.org/57007/diff/1007/1009#newcode444
Line 444: RefPtr<CSSRuleList> matched = window->getMatchedCSSRules(element, "",
authorOnly);
nit: please respect 80 chars per line

http://codereview.chromium.org/57007/diff/1007/1009#newcode445
Line 445: for (unsigned i = 0; i < matched->length(); i++) {
nit: we generally do ++i

http://codereview.chromium.org/57007/diff/1007/1009#newcode453
Line 453: if (CSSMutableStyleDeclaration* style = rule->style())
This should not pass our lint audits: please avoid using assignments in the
boolean expression

http://codereview.chromium.org/57007/diff/1007/1009#newcode481
Line 481: String name = attr->name().toString();
nit: indent

http://codereview.chromium.org/57007/diff/1007/1010
File webkit/glue/devtools/dom_agent_impl.h (right):

http://codereview.chromium.org/57007/diff/1007/1010#newcode44
Line 44: void GetNodeStyles(int call_id, int id, bool authorOnly);
author_only

http://codereview.chromium.org/57007/diff/1007/1011
File webkit/glue/devtools/js/devtools.js (right):

http://codereview.chromium.org/57007/diff/1007/1011#newcode113
Line 113: function callback(styles) {
we generally use inner functions as follows:
var callback = function(styles) {
};

http://codereview.chromium.org/57007/diff/1007/1011#newcode119
Line 119: domAgent.getNodeStylesAsync(node, !Preferences.showUserAgentStyles,
callback);
80 chars

http://codereview.chromium.org/57007/diff/1007/1012
File webkit/glue/devtools/js/devtools_host_stub.js (right):

http://codereview.chromium.org/57007/diff/1007/1012#newcode100
Line 100: RemoteDomAgent.DidGetNodeStyles(callId, ["display: none", "display:
none", [["attr", "val", "display: none"]], []]);
80 chars

http://codereview.chromium.org/57007/diff/1007/1012#newcode100
Line 100: RemoteDomAgent.DidGetNodeStyles(callId, ["display: none", "display:
none", [["attr", "val", "display: none"]], []]);
can you put more meaningful data here for the sake of manual testing?

http://codereview.chromium.org/57007/diff/1007/1013
File webkit/glue/devtools/js/dom_agent.js (right):

http://codereview.chromium.org/57007/diff/1007/1013#newcode461
Line 461: * Asyncronously requests all the information about styles for the
node:
... node.

http://codereview.chromium.org/57007/diff/1007/1013#newcode462
Line 462: * @param {devtools.DomNode} node
... Node to get styles for.

http://codereview.chromium.org/57007/diff/1007/1013#newcode463
Line 463: * @param {boolean} authorOnly is a parameter for getMatchedCSSRules
authorOnly Determines blah-blah-blah ... And what is authorOnly after all?
Little doc about Author vs User styles is appreciated.

http://codereview.chromium.org/57007/diff/1007/1013#newcode477
Line 477: devtools.DomAgent.prototype.getNodeStylesCallback_ = function(node,
callback, styles) {
here and below: 80 chars

http://codereview.chromium.org/57007/diff/1007/1013#newcode477
Line 477: devtools.DomAgent.prototype.getNodeStylesCallback_ = function(node,
callback, styles) {
JsDoc?

http://codereview.chromium.org/57007/diff/1007/1013#newcode510
Line 510: * @param {devtools.DomNode} node ("disabled" atyle prperties are
stored there).
_s_tyle, no parenthesis. stored -> passed

http://codereview.chromium.org/57007/diff/1007/1013#newcode511
Line 511: * @param {string} computedStyle is a cssText of result of
getComputedStyle().
Please use proper JsDoc style of parameters here and below; i.e.
@param {string} computedStyle Textual result of teh getComputedStyle invocation.

http://codereview.chromium.org/57007/diff/1007/1013#newcode517
Line 517: *     getMatchedCSSRules(node, "", authorOnly). Each elemet consists
of:
Using dictionary instead of the list would simplify this.

http://codereview.chromium.org/57007/diff/1007/1013#newcode545
Line 545: if (descr.length > 2)
Looks like using dictionary for these would be more elegant again.

http://codereview.chromium.org/57007/diff/1007/1013#newcode557
Line 557: * Creates a style object by cssText for representing a snapshot of the
style.
Please fix typos and style

http://codereview.chromium.org/57007/diff/1007/1013#newcode571
Line 571: style.__defineGetter__("__disabledPropertyPriorities",
We don't need to artificially define getters - we are controlling the
devtools.DomNode definition itself. Just add a getter there.

http://codereview.chromium.org/57007/diff/1007/1013#newcode584
Line 584: devtools.DomAgent.NodeStyles.prototype.getDisabledProperties_ =
function(styleId) {
Just move this to DomNode.

[SeRya, 2009-03-30 17:31:04]

http://codereview.chromium.org/57007/diff/1007/1008
File webkit/glue/devtools/dom_agent.h (right):

http://codereview.chromium.org/57007/diff/1007/1008#newcode33
Line 33: /* Requests that the element's style information are sent to the
client. */ \
On 2009/03/30 12:12:44, pfeldman wrote:
> nit: ... is sent to the ...

Done.

http://codereview.chromium.org/57007/diff/1007/1008#newcode45
Line 45: METHOD2(DidGetNodeStyles, int /* call_id */, Value /* styles */) \
On 2009/03/30 12:12:44, pfeldman wrote:
> Please preserve method order as in direct calls.

Done.

http://codereview.chromium.org/57007/diff/1007/1009
File webkit/glue/devtools/dom_agent_impl.cc (right):

http://codereview.chromium.org/57007/diff/1007/1009#newcode429
Line 429: OwnPtr<ListValue> style_attributes(new ListValue());
On 2009/03/30 12:12:44, pfeldman wrote:
> Given that this is not performance-critical operation, I would prefer
Dictionary
> with named properties over the list. See how this is done in the net_agent.

Done.

http://codereview.chromium.org/57007/diff/1007/1009#newcode433
Line 433: if (node && (node->nodeType() == Node::ELEMENT_NODE)) {
On 2009/03/30 12:12:44, pfeldman wrote:
> given the size of the branch, I would suggest to replace nesting with guard
> expression. i.e.
> if (!node || node->nodeType() != ...) {
>   DidGetNodeStyles(call_id, styles);
>   return;
> }
> 
> ... rest of the code ...

I extracted the DescribeCSSRules and DescribeAttributes methods instead.

http://codereview.chromium.org/57007/diff/1007/1009#newcode439
Line 439: DOMWindow* window = document ? document->defaultView() : NULL;
On 2009/03/30 12:12:44, pfeldman wrote:
> I don't think document can be null. Can it?

It can't

http://codereview.chromium.org/57007/diff/1007/1009#newcode444
Line 444: RefPtr<CSSRuleList> matched = window->getMatchedCSSRules(element, "",
authorOnly);
On 2009/03/30 12:12:44, pfeldman wrote:
> nit: please respect 80 chars per line

Done.

http://codereview.chromium.org/57007/diff/1007/1009#newcode445
Line 445: for (unsigned i = 0; i < matched->length(); i++) {
On 2009/03/30 12:12:44, pfeldman wrote:
> nit: we generally do ++i

Done.

http://codereview.chromium.org/57007/diff/1007/1010
File webkit/glue/devtools/dom_agent_impl.h (right):

http://codereview.chromium.org/57007/diff/1007/1010#newcode44
Line 44: void GetNodeStyles(int call_id, int id, bool authorOnly);
On 2009/03/30 12:12:44, pfeldman wrote:
> author_only

Done.

http://codereview.chromium.org/57007/diff/1007/1011
File webkit/glue/devtools/js/devtools.js (right):

http://codereview.chromium.org/57007/diff/1007/1011#newcode113
Line 113: function callback(styles) {
On 2009/03/30 12:12:44, pfeldman wrote:
> we generally use inner functions as follows:
> var callback = function(styles) {
> };
> 

Done.

http://codereview.chromium.org/57007/diff/1007/1012
File webkit/glue/devtools/js/devtools_host_stub.js (right):

http://codereview.chromium.org/57007/diff/1007/1012#newcode100
Line 100: RemoteDomAgent.DidGetNodeStyles(callId, ["display: none", "display:
none", [["attr", "val", "display: none"]], []]);
On 2009/03/30 12:12:44, pfeldman wrote:
> 80 chars

Done.

http://codereview.chromium.org/57007/diff/1007/1013
File webkit/glue/devtools/js/dom_agent.js (right):

http://codereview.chromium.org/57007/diff/1007/1013#newcode461
Line 461: * Asyncronously requests all the information about styles for the
node:
On 2009/03/30 12:12:44, pfeldman wrote:
> ... node.

Done.

http://codereview.chromium.org/57007/diff/1007/1013#newcode462
Line 462: * @param {devtools.DomNode} node
On 2009/03/30 12:12:44, pfeldman wrote:
> ... Node to get styles for.

Done.

http://codereview.chromium.org/57007/diff/1007/1013#newcode477
Line 477: devtools.DomAgent.prototype.getNodeStylesCallback_ = function(node,
callback, styles) {
On 2009/03/30 12:12:44, pfeldman wrote:
> here and below: 80 chars

Done.

http://codereview.chromium.org/57007/diff/1007/1013#newcode477
Line 477: devtools.DomAgent.prototype.getNodeStylesCallback_ = function(node,
callback, styles) {
On 2009/03/30 12:12:44, pfeldman wrote:
> JsDoc?

Done.

http://codereview.chromium.org/57007/diff/1007/1013#newcode517
Line 517: *     getMatchedCSSRules(node, "", authorOnly). Each elemet consists
of:
On 2009/03/30 12:12:44, pfeldman wrote:
> Using dictionary instead of the list would simplify this.

Done.

[pfeldman, 2009-03-30 20:44:09]

http://codereview.chromium.org/57007/diff/21/1030
File webkit/glue/devtools/dom_agent.h (right):

http://codereview.chromium.org/57007/diff/21/1030#newcode14
Line 14: #define DOM_AGENT_STRUCT(METHOD0, METHOD1, METHOD2, METHOD3) \
FYI: this needs to be merged with the current HEAD.

http://codereview.chromium.org/57007/diff/21/1030#newcode34
Line 34: METHOD3(GetNodeStyles, int /* call_id */, int /* id */, bool /*
author_only */) \
80 chars

http://codereview.chromium.org/57007/diff/21/1031
File webkit/glue/devtools/dom_agent_impl.cc (right):

http://codereview.chromium.org/57007/diff/21/1031#newcode14
Line 14: #include "DOMWindow.h"
Capital 'O' goes first

http://codereview.chromium.org/57007/diff/21/1031#newcode424
Line 424: void DomAgentImpl::GetNodeStyles(int call_id, int element_id, bool
author_only) {
80 chars

http://codereview.chromium.org/57007/diff/21/1031#newcode437
Line 437: if (window) {
can window be null here?

http://codereview.chromium.org/57007/diff/21/1031#newcode450
Line 450: webkit_glue::StringToStdWString(computed_style));
Here and below: using webkit_glue::StringToStdString will save you tow
WideToUTF8/UTF8ToWide conversions.

http://codereview.chromium.org/57007/diff/21/1031#newcode465
Line 465: if (!matched.item(i)->isStyleRule())
nit: here and below, we agreed to use {} even for one line ifs in devtools. Not
all code is yet formatted so, but we now try to be consistent.

http://codereview.chromium.org/57007/diff/21/1031#newcode505
Line 505: webkit_glue::StringToStdWString(attr->name().toString());
indent

http://codereview.chromium.org/57007/diff/21/1032
File webkit/glue/devtools/dom_agent_impl.h (right):

http://codereview.chromium.org/57007/diff/21/1032#newcode124
Line 124: void DescribeCSSRules(WebCore::CSSRuleList& matched,
These are called "BuildValueFor*" above and are implemented as factories for
ListValue. Could you rename these to be consistent with the rest of the code?

http://codereview.chromium.org/57007/diff/21/1033
File webkit/glue/devtools/js/devtools.js (right):

http://codereview.chromium.org/57007/diff/21/1033#newcode110
Line 110: if (node && node.nodeType !== Node.ELEMENT_NODE)
This means that the GetNodeStyles is only getting ELEMENT_NODE nodes. In this
case please put DCHECK into the C++ code and assume that you are always getting
ELEMENT node s there.

http://codereview.chromium.org/57007/diff/21/1033#newcode111
Line 111: node = null;
Why not to put lines 121-122 here and return? There seems to be no reason for
creating callback object.

http://codereview.chromium.org/57007/diff/21/1033#newcode119
Line 119: domAgent.getNodeStylesAsync(node, !Preferences.showUserAgentStyles,
callback);
80 chars

http://codereview.chromium.org/57007/diff/21/1034
File webkit/glue/devtools/js/devtools_host_stub.js (right):

http://codereview.chromium.org/57007/diff/21/1034#newcode100
Line 100: computedStyle: "display: none",
please use 4 space indent

http://codereview.chromium.org/57007/diff/21/1035
File webkit/glue/devtools/js/dom_agent.js (right):

http://codereview.chromium.org/57007/diff/21/1035#newcode60
Line 60: this.diablesStyleProperties_ = null;
"diables" or "disabled"?

http://codereview.chromium.org/57007/diff/21/1035#newcode83
Line 83: get style() {return this.node_.styles_.attributes[this.name];}
why not to do
this.attributesMap_[name].style = foo;
in node.setStyles_ instead?
I see some benefit in doing everything through styles_, but it makes the code
more complex. For example attributes are not only set using this method, so you
are going to fail using this approach.

http://codereview.chromium.org/57007/diff/21/1035#newcode188
Line 188: devtools.DomNode.prototype.__defineGetter__("style", function() {
same here - why not to assign "style" to what we need in setStyles_ for
simplicity?

http://codereview.chromium.org/57007/diff/21/1035#newcode200
Line 200: * @param {string} computedStyle is a cssText of result of
getComputedStyle().
Please use proper JsDoc style as was suggested earlier here and below.

http://codereview.chromium.org/57007/diff/21/1035#newcode201
Line 201: * @param {strung} inlineStyle is a style.cssText (defined in the STYLE
strung?

http://codereview.chromium.org/57007/diff/21/1035#newcode203
Line 203: * @param {object} styleAttributes represents represents 'style'
property
{Object} from capital O
"represents represents"

http://codereview.chromium.org/57007/diff/21/1035#newcode209
Line 209: * @constructor
constructor?

http://codereview.chromium.org/57007/diff/21/1035#newcode239
Line 239: styles.matchedCSSRules.push({selectorText: selector, style: style,
nit: i try not to use structures like style: style to avoid confusion. 'style' :
style sounds more readable.

http://codereview.chromium.org/57007/diff/21/1035#newcode248
Line 248: * Creates a style object by cssText for representing a snapshot of the
style.
... from cssText

http://codereview.chromium.org/57007/diff/21/1035#newcode259
Line 259: var style = document.createElement("SPAN").style;
why are you creating a dummy span object? this one does look like a hack. It
would be great if server was sending the structure that has all the information
needed for style demonstration.

http://codereview.chromium.org/57007/diff/21/1035#newcode262
Line 262: style.__defineGetter__("__disabledPropertyPriorities",
I thought that we agreed that we can simply assign value here. can't we?

http://codereview.chromium.org/57007/diff/21/1035#newcode313
Line 313: devtools.DomDocument.prototype.defaultView = {};
I think having an explicit DomWindow class would read better.

[SeRya, 2009-03-31 14:51:20]

http://codereview.chromium.org/57007/diff/21/1030
File webkit/glue/devtools/dom_agent.h (right):

http://codereview.chromium.org/57007/diff/21/1030#newcode14
Line 14: #define DOM_AGENT_STRUCT(METHOD0, METHOD1, METHOD2, METHOD3) \
On 2009/03/30 20:44:09, pfeldman wrote:
> FYI: this needs to be merged with the current HEAD.

Done.

http://codereview.chromium.org/57007/diff/21/1030#newcode34
Line 34: METHOD3(GetNodeStyles, int /* call_id */, int /* id */, bool /*
author_only */) \
On 2009/03/30 20:44:09, pfeldman wrote:
> 80 chars

Done.

http://codereview.chromium.org/57007/diff/21/1031
File webkit/glue/devtools/dom_agent_impl.cc (right):

http://codereview.chromium.org/57007/diff/21/1031#newcode14
Line 14: #include "DOMWindow.h"
On 2009/03/30 20:44:09, pfeldman wrote:
> Capital 'O' goes first

Done.

http://codereview.chromium.org/57007/diff/21/1031#newcode424
Line 424: void DomAgentImpl::GetNodeStyles(int call_id, int element_id, bool
author_only) {
On 2009/03/30 20:44:09, pfeldman wrote:
> 80 chars

Done.

http://codereview.chromium.org/57007/diff/21/1031#newcode437
Line 437: if (window) {
On 2009/03/30 20:44:09, pfeldman wrote:
> can window be null here?

It shouldn't because since a node detached from a document or window it must be
removed form the id map.

http://codereview.chromium.org/57007/diff/21/1031#newcode450
Line 450: webkit_glue::StringToStdWString(computed_style));
On 2009/03/30 20:44:09, pfeldman wrote:
> Here and below: using webkit_glue::StringToStdString will save you tow
> WideToUTF8/UTF8ToWide conversions.

Done.

http://codereview.chromium.org/57007/diff/21/1031#newcode465
Line 465: if (!matched.item(i)->isStyleRule())
On 2009/03/30 20:44:09, pfeldman wrote:
> nit: here and below, we agreed to use {} even for one line ifs in devtools.
Not
> all code is yet formatted so, but we now try to be consistent.

Done.

http://codereview.chromium.org/57007/diff/21/1031#newcode505
Line 505: webkit_glue::StringToStdWString(attr->name().toString());
On 2009/03/30 20:44:09, pfeldman wrote:
> indent

Done.

http://codereview.chromium.org/57007/diff/21/1032
File webkit/glue/devtools/dom_agent_impl.h (right):

http://codereview.chromium.org/57007/diff/21/1032#newcode124
Line 124: void DescribeCSSRules(WebCore::CSSRuleList& matched,
On 2009/03/30 20:44:09, pfeldman wrote:
> These are called "BuildValueFor*" above and are implemented as factories for
> ListValue. Could you rename these to be consistent with the rest of the code?

Done.

http://codereview.chromium.org/57007/diff/21/1033
File webkit/glue/devtools/js/devtools.js (right):

http://codereview.chromium.org/57007/diff/21/1033#newcode110
Line 110: if (node && node.nodeType !== Node.ELEMENT_NODE)
On 2009/03/30 20:44:09, pfeldman wrote:
> This means that the GetNodeStyles is only getting ELEMENT_NODE nodes. In this
> case please put DCHECK into the C++ code and assume that you are always
getting
> ELEMENT node s there.

Done.

http://codereview.chromium.org/57007/diff/21/1033#newcode111
Line 111: node = null;
On 2009/03/30 20:44:09, pfeldman wrote:
> Why not to put lines 121-122 here and return? There seems to be no reason for
> creating callback object.

Done.

http://codereview.chromium.org/57007/diff/21/1033#newcode119
Line 119: domAgent.getNodeStylesAsync(node, !Preferences.showUserAgentStyles,
callback);
On 2009/03/30 20:44:09, pfeldman wrote:
> 80 chars

Done.

http://codereview.chromium.org/57007/diff/21/1034
File webkit/glue/devtools/js/devtools_host_stub.js (right):

http://codereview.chromium.org/57007/diff/21/1034#newcode100
Line 100: computedStyle: "display: none",
On 2009/03/30 20:44:09, pfeldman wrote:
> please use 4 space indent

Done.

http://codereview.chromium.org/57007/diff/21/1035
File webkit/glue/devtools/js/dom_agent.js (right):

http://codereview.chromium.org/57007/diff/21/1035#newcode60
Line 60: this.diablesStyleProperties_ = null;
On 2009/03/30 20:44:09, pfeldman wrote:
> "diables" or "disabled"?

Done.

http://codereview.chromium.org/57007/diff/21/1035#newcode83
Line 83: get style() {return this.node_.styles_.attributes[this.name];}
On 2009/03/30 20:44:09, pfeldman wrote:
> why not to do
> this.attributesMap_[name].style = foo;
> in node.setStyles_ instead?
> I see some benefit in doing everything through styles_, but it makes the code
> more complex. For example attributes are not only set using this method, so
you
> are going to fail using this approach.

Attributes could be updated. Disabled properties should survive the update.
Anyway, looking into other ways to update styles is a good point.

http://codereview.chromium.org/57007/diff/21/1035#newcode188
Line 188: devtools.DomNode.prototype.__defineGetter__("style", function() {
On 2009/03/30 20:44:09, pfeldman wrote:
> same here - why not to assign "style" to what we need in setStyles_ for
> simplicity?

It means to move the complexity from one place to another (in this case - to
clearing up code). I see no benefits.

http://codereview.chromium.org/57007/diff/21/1035#newcode200
Line 200: * @param {string} computedStyle is a cssText of result of
getComputedStyle().
On 2009/03/30 20:44:09, pfeldman wrote:
> Please use proper JsDoc style as was suggested earlier here and below.

What is improper JsDoc style here?

http://codereview.chromium.org/57007/diff/21/1035#newcode203
Line 203: * @param {object} styleAttributes represents represents 'style'
property
On 2009/03/30 20:44:09, pfeldman wrote:
> {Object} from capital O
> "represents represents"

Done.

http://codereview.chromium.org/57007/diff/21/1035#newcode209
Line 209: * @constructor
On 2009/03/30 20:44:09, pfeldman wrote:
> constructor?

Done.

http://codereview.chromium.org/57007/diff/21/1035#newcode239
Line 239: styles.matchedCSSRules.push({selectorText: selector, style: style,
On 2009/03/30 20:44:09, pfeldman wrote:
> nit: i try not to use structures like style: style to avoid confusion. 'style'
:
> style sounds more readable.

Done.

http://codereview.chromium.org/57007/diff/21/1035#newcode248
Line 248: * Creates a style object by cssText for representing a snapshot of the
style.
On 2009/03/30 20:44:09, pfeldman wrote:
> ... from cssText

Done.

http://codereview.chromium.org/57007/diff/21/1035#newcode259
Line 259: var style = document.createElement("SPAN").style;
On 2009/03/30 20:44:09, pfeldman wrote:
> why are you creating a dummy span object? this one does look like a hack. It
> would be great if server was sending the structure that has all the
information
> needed for style demonstration.

To avoid implementing the CSSStyleDeclaration interface at this stage. It seems
to be easies way to serialize and deserialize CSS styles.

http://codereview.chromium.org/57007/diff/21/1035#newcode262
Line 262: style.__defineGetter__("__disabledPropertyPriorities",
On 2009/03/30 20:44:09, pfeldman wrote:
> I thought that we agreed that we can simply assign value here. can't we?

Done.

http://codereview.chromium.org/57007/diff/21/1035#newcode313
Line 313: devtools.DomDocument.prototype.defaultView = {};
On 2009/03/30 20:44:09, pfeldman wrote:
> I think having an explicit DomWindow class would read better.

Done.

[pfeldman, 2009-03-31 16:23:52]

http://codereview.chromium.org/57007/diff/21/1034
File webkit/glue/devtools/js/devtools_host_stub.js (right):

http://codereview.chromium.org/57007/diff/21/1034#newcode100
Line 100: computedStyle: "display: none",
On 2009/03/31 14:51:20, SeRya wrote:
> On 2009/03/30 20:44:09, pfeldman wrote:
> > please use 4 space indent
> 
> Done.

Actually, I fooled you - Yury found out that it should be 2 spaces. This is nit
however, so we can fix it later.

http://codereview.chromium.org/57007/diff/21/1035
File webkit/glue/devtools/js/dom_agent.js (right):

http://codereview.chromium.org/57007/diff/21/1035#newcode188
Line 188: devtools.DomNode.prototype.__defineGetter__("style", function() {
On 2009/03/31 14:51:20, SeRya wrote:
> On 2009/03/30 20:44:09, pfeldman wrote:
> > same here - why not to assign "style" to what we need in setStyles_ for
> > simplicity?
> 
> It means to move the complexity from one place to another (in this case - to
> clearing up code). I see no benefits.

The benefit is not to use 'getter functions' and constructs like
'__defineGetter__' that are unnecessary. And I don't see where my approach is
adding complexity - I think it only removes it and makes things straightforward.

http://codereview.chromium.org/57007/diff/21/1035#newcode200
Line 200: * @param {string} computedStyle is a cssText of result of
getComputedStyle().
On 2009/03/31 14:51:20, SeRya wrote:
> On 2009/03/30 20:44:09, pfeldman wrote:
> > Please use proper JsDoc style as was suggested earlier here and below.
> 
> What is improper JsDoc style here?

http://jsdoc.sourceforge.net/#tagref

http://codereview.chromium.org/57007/diff/21/1035#newcode259
Line 259: var style = document.createElement("SPAN").style;
On 2009/03/31 14:51:20, SeRya wrote:
> On 2009/03/30 20:44:09, pfeldman wrote:
> > why are you creating a dummy span object? this one does look like a hack. It
> > would be great if server was sending the structure that has all the
> information
> > needed for style demonstration.
> 
> To avoid implementing the CSSStyleDeclaration interface at this stage. It
seems
> to be easies way to serialize and deserialize CSS styles.

But this style is going to be instantiated in a different context (in the
inspector, not in the inspectable page). What is the complexity behind
implementing CSSStyleDeclaration?

http://codereview.chromium.org/57007/diff/1053/45
File webkit/glue/devtools/dom_agent_impl.cc (right):

http://codereview.chromium.org/57007/diff/1053/45#newcode453
Line 453: DCHECK(node && node->nodeType() == Node::ELEMENT_NODE);
You should assume that if this node exists, then it is element node. But you
should not assume it exists at all times. Given the asynchronous nature of the
interaction, node could disappear while CSS request was going over IPC.

http://codereview.chromium.org/57007/diff/1053/45#newcode456
Line 456: 
nit: why are you typing on every other line?

http://codereview.chromium.org/57007/diff/1053/45#newcode530
Line 530: webkit_glue::StringToStdWString(attr->name().toString());
Please use StringToStdString as was noted before.

http://codereview.chromium.org/57007/diff/1053/47
File webkit/glue/devtools/js/devtools.js (right):

http://codereview.chromium.org/57007/diff/1053/47#newcode190
Line 190: {
nit: mixed style for opening bracket.

http://codereview.chromium.org/57007/diff/1053/49
File webkit/glue/devtools/js/dom_agent.js (right):

http://codereview.chromium.org/57007/diff/1053/49#newcode60
Line 60: this.diabledStyleProperties_ = {};
I still think there is a typo here.

http://codereview.chromium.org/57007/diff/1053/49#newcode228
Line 228: get style() {
I still don't like this a lot. Your comment actually proves that this is a hack.

http://codereview.chromium.org/57007/diff/1053/49#newcode404
Line 404: devtools.DomDocument.prototype.defaultView = new devtools.DomWindow();
if you look at the inspector_controller_impl.js, you will see that there is
already a window object there. Would be great if you could combine those using
your new class.

[SeRya, 2009-04-01 09:20:12]

http://codereview.chromium.org/57007/diff/1053/45
File webkit/glue/devtools/dom_agent_impl.cc (right):

http://codereview.chromium.org/57007/diff/1053/45#newcode453
Line 453: DCHECK(node && node->nodeType() == Node::ELEMENT_NODE);
On 2009/03/31 16:23:53, pfeldman wrote:
> You should assume that if this node exists, then it is element node. But you
> should not assume it exists at all times. Given the asynchronous nature of the
> interaction, node could disappear while CSS request was going over IPC.

Done.

http://codereview.chromium.org/57007/diff/1053/45#newcode456
Line 456: 
On 2009/03/31 16:23:53, pfeldman wrote:
> nit: why are you typing on every other line?

Done.

http://codereview.chromium.org/57007/diff/1053/45#newcode530
Line 530: webkit_glue::StringToStdWString(attr->name().toString());
On 2009/03/31 16:23:53, pfeldman wrote:
> Please use StringToStdString as was noted before.

the first argument must be a wstring

http://codereview.chromium.org/57007/diff/1053/47
File webkit/glue/devtools/js/devtools.js (right):

http://codereview.chromium.org/57007/diff/1053/47#newcode190
Line 190: {
On 2009/03/31 16:23:53, pfeldman wrote:
> nit: mixed style for opening bracket.

Done.

http://codereview.chromium.org/57007/diff/1053/49
File webkit/glue/devtools/js/dom_agent.js (right):

http://codereview.chromium.org/57007/diff/1053/49#newcode60
Line 60: this.diabledStyleProperties_ = {};
On 2009/03/31 16:23:53, pfeldman wrote:
> I still think there is a typo here.

Done.

http://codereview.chromium.org/57007/diff/1053/49#newcode228
Line 228: get style() {
On 2009/03/31 16:23:53, pfeldman wrote:
> I still don't like this a lot. Your comment actually proves that this is a
hack.

I still didn't get why your way to hack is better than mine. Doing so I avoid
additional work when attributes are updated. Anyway I suggest keeping it as is
for now and refactoring later (since it is not final CL about styles pane).

http://codereview.chromium.org/57007/diff/1053/49#newcode404
Line 404: devtools.DomDocument.prototype.defaultView = new devtools.DomWindow();
On 2009/03/31 16:23:53, pfeldman wrote:
> if you look at the inspector_controller_impl.js, you will see that there is
> already a window object there. Would be great if you could combine those using
> your new class.

Done.

[pfeldman, 2009-04-01 10:01:54]

http://codereview.chromium.org/57007/diff/3001/1057
File webkit/glue/devtools/dom_agent_impl.cc (right):

http://codereview.chromium.org/57007/diff/3001/1057#newcode581
Line 581: for (unsigned i = 0; i < numAttrs; ++i) {
this should also be merged a bit due to rename requested by Darin recently.

http://codereview.chromium.org/57007/diff/3001/1058
File webkit/glue/devtools/dom_agent_impl.h (right):

http://codereview.chromium.org/57007/diff/3001/1058#newcode23
Line 23: class NameAttrMap;
I am afraid you need to merge this again since now NamedNodeMap should be used
(WebKit update).

http://codereview.chromium.org/57007/diff/3001/1061
File webkit/glue/devtools/js/dom_agent.js (right):

http://codereview.chromium.org/57007/diff/3001/1061#newcode330
Line 330: var style = document.createElement("SPAN").style;
I think you owe me a comment on this one.

http://codereview.chromium.org/57007/diff/3001/1061#newcode827
Line 827: node.clearStyles_();
Given that clearStyles_ will make the temporary object disappear, what would
happen on the subsequent attr.style call? Or is this call illegal?

[SeRya, 2009-04-01 12:54:07]

Merged

http://codereview.chromium.org/57007/diff/3001/1061
File webkit/glue/devtools/js/dom_agent.js (right):

http://codereview.chromium.org/57007/diff/3001/1061#newcode827
Line 827: node.clearStyles_();
On 2009/04/01 10:01:54, pfeldman wrote:
> Given that clearStyles_ will make the temporary object disappear, what would
> happen on the subsequent attr.style call? Or is this call illegal?

Yes, it's illegal.

[pfeldman, 2009-04-01 13:16:21]

LGTM given comments are fixed

http://codereview.chromium.org/57007/diff/1076/3022
File webkit/glue/devtools/dom_agent_impl.cc (right):

http://codereview.chromium.org/57007/diff/1076/3022#newcode582
Line 582: unsigned numAttrs = attr_map->length();
please do not revert changes made by others

http://codereview.chromium.org/57007/diff/1076/3023
File webkit/glue/devtools/dom_agent_impl.h (right):

http://codereview.chromium.org/57007/diff/1076/3023#newcode134
Line 134: DictionaryValue& description);
Forward declaration for DictionaryValue is missing.

[SeRya, 2009-04-01 13:26:04]

http://codereview.chromium.org/57007/diff/1076/3022
File webkit/glue/devtools/dom_agent_impl.cc (right):

http://codereview.chromium.org/57007/diff/1076/3022#newcode582
Line 582: unsigned numAttrs = attr_map->length();
On 2009/04/01 13:16:21, pfeldman wrote:
> please do not revert changes made by others

Done.

http://codereview.chromium.org/57007/diff/1076/3023
File webkit/glue/devtools/dom_agent_impl.h (right):

http://codereview.chromium.org/57007/diff/1076/3023#newcode134
Line 134: DictionaryValue& description);
On 2009/04/01 13:16:21, pfeldman wrote:
> Forward declaration for DictionaryValue is missing.

Done.

[SeRya, 2009-04-01 20:15:15]

Merged

[SeRya, 2009-04-02 13:17:05]

Merge