DevTools: Do not force style sheets update on inspector start.

DevTools: Do not force style sheets update on inspector start.

This also fixes the case case when inspector was closed and then stylesheet was removed on the inspected page
In this case inspector frontend received an incorrect styleSheetRemoved event after reopening.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164379

[vsevik, 2013-12-26 15:10:12]

PTAL

[apavlov, 2013-12-27 07:12:30]

lgtm with comments

https://codereview.chromium.org/121263002/diff/30001/LayoutTests/http/tests/i...
File LayoutTests/http/tests/inspector-protocol/resources/protocol-test.js
(right):

https://codereview.chromium.org/121263002/diff/30001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector-protocol/resources/protocol-test.js:96: var
inspectorFrontend =
window.internals.openDummyInspectorFrontend(dummyFrontendURL);
dummyFrontendURL could as well be an argument to openInspector, since it is not
used anywhere else, and globals are evil, as we know :)

https://codereview.chromium.org/121263002/diff/30001/LayoutTests/inspector-pr...
File LayoutTests/inspector-protocol/stylesheet-tracking-restart.html (right):

https://codereview.chromium.org/121263002/diff/30001/LayoutTests/inspector-pr...
LayoutTests/inspector-protocol/stylesheet-tracking-restart.html:35:
styleElement1 = createStyleSheet("body.class1 { color: red; } \n /*#
sourceURL=foo.js */");
.css ?

https://codereview.chromium.org/121263002/diff/30001/LayoutTests/inspector-pr...
LayoutTests/inspector-protocol/stylesheet-tracking-restart.html:36:
styleElement2 = createStyleSheet("body.class2 { color: green; } \n /*#
sourceURL=bar.js */");
ditto

https://codereview.chromium.org/121263002/diff/30001/Source/core/inspector/In...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/121263002/diff/30001/Source/core/inspector/In...
Source/core/inspector/InspectorCSSAgent.cpp:865: bool isNew = initial ||
!m_cssStyleSheetToInspectorStyleSheet.contains(*it);
The var name does not seem to fully reflect the idea behind it. It's rather
"shouldAdd" or something similar (that is, the stylesheet might not be new, but
should be added to the frontend and, optionally, to the backend (if it's really
new)).

[lushnikov, 2013-12-27 08:20:16]

https://codereview.chromium.org/121263002/diff/30001/LayoutTests/inspector-pr...
File LayoutTests/inspector-protocol/stylesheet-tracking-restart.html (right):

https://codereview.chromium.org/121263002/diff/30001/LayoutTests/inspector-pr...
LayoutTests/inspector-protocol/stylesheet-tracking-restart.html:12:
document.body.appendChild(styleElement);
let's add/remove stylesheets to document.head instead

https://codereview.chromium.org/121263002/diff/30001/Source/core/inspector/In...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/121263002/diff/30001/Source/core/inspector/In...
Source/core/inspector/InspectorCSSAgent.cpp:834: void
InspectorCSSAgent::updateActiveStyleSheets(Document* document, const
Vector<CSSStyleSheet*>& allSheetsVector, bool initial)
nit: consider using default value

bool initial = false)

[apavlov, 2013-12-27 08:27:39]

https://codereview.chromium.org/121263002/diff/30001/Source/core/inspector/In...
File Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/121263002/diff/30001/Source/core/inspector/In...
Source/core/inspector/InspectorCSSAgent.cpp:834: void
InspectorCSSAgent::updateActiveStyleSheets(Document* document, const
Vector<CSSStyleSheet*>& allSheetsVector, bool initial)
On 2013/12/27 08:20:17, lushnikov wrote:
> nit: consider using default value
> 
> bool initial = false)

Default argument values are disproved in Blink, according to pfeldman.

[eseidel, 2013-12-27 08:35:17]

I can't comment as to pfeldman's specific meaning (as I'm not pfeldman), but my
2¢:  I guess default arguments are less clear than explicit ones.  We certainly
have a lot of them.

I would be more apt about commenting about bools as arguments.  Whenever
possible I try to encourage patches to use enums instead of bools as reading:
foo(true, false, true) is neigh impossible, but:
foo(StrictMode, DisableExceptions, EnableLogging) is much more clear. :)

[yurys, 2013-12-27 08:43:06]

https://codereview.chromium.org/121263002/diff/30001/LayoutTests/http/tests/i...
File LayoutTests/http/tests/inspector-protocol/resources/protocol-test.js
(right):

https://codereview.chromium.org/121263002/diff/30001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector-protocol/resources/protocol-test.js:96: var
inspectorFrontend =
window.internals.openDummyInspectorFrontend(dummyFrontendURL);
On 2013/12/27 07:12:30, apavlov wrote:
> dummyFrontendURL could as well be an argument to openInspector, since it is
not
> used anywhere else, and globals are evil, as we know :)

+1, please pass front-end URL as an explicit parameter.

[vsevik, 2013-12-27 14:52:45]

On 2013/12/27 08:20:16, lushnikov wrote:
>
https://codereview.chromium.org/121263002/diff/30001/LayoutTests/inspector-pr...
> File LayoutTests/inspector-protocol/stylesheet-tracking-restart.html (right):
> 
>
https://codereview.chromium.org/121263002/diff/30001/LayoutTests/inspector-pr...
> LayoutTests/inspector-protocol/stylesheet-tracking-restart.html:12:
> document.body.appendChild(styleElement);
> let's add/remove stylesheets to document.head instead
Done.

>
https://codereview.chromium.org/121263002/diff/30001/Source/core/inspector/In...
> File Source/core/inspector/InspectorCSSAgent.cpp (right):
> 
>
https://codereview.chromium.org/121263002/diff/30001/Source/core/inspector/In...
> Source/core/inspector/InspectorCSSAgent.cpp:834: void
> InspectorCSSAgent::updateActiveStyleSheets(Document* document, const
> Vector<CSSStyleSheet*>& allSheetsVector, bool initial)
> nit: consider using default value
> 
> bool initial = false)
Not done per comments below.

[vsevik, 2013-12-27 14:52:58]

On 2013/12/27 08:35:17, eseidel wrote:
> I can't comment as to pfeldman's specific meaning (as I'm not pfeldman), but
my
> 2¢:  I guess default arguments are less clear than explicit ones.  We
certainly
> have a lot of them.
> 
> I would be more apt about commenting about bools as arguments.  Whenever
> possible I try to encourage patches to use enums instead of bools as reading:
> foo(true, false, true) is neigh impossible, but:
> foo(StrictMode, DisableExceptions, EnableLogging) is much more clear. :)

Done

[vsevik, 2013-12-27 14:53:32]

On 2013/12/27 08:43:06, yurys wrote:
>
https://codereview.chromium.org/121263002/diff/30001/LayoutTests/http/tests/i...
> File LayoutTests/http/tests/inspector-protocol/resources/protocol-test.js
> (right):
> 
>
https://codereview.chromium.org/121263002/diff/30001/LayoutTests/http/tests/i...
> LayoutTests/http/tests/inspector-protocol/resources/protocol-test.js:96: var
> inspectorFrontend =
> window.internals.openDummyInspectorFrontend(dummyFrontendURL);
> On 2013/12/27 07:12:30, apavlov wrote:
> > dummyFrontendURL could as well be an argument to openInspector, since it is
> not
> > used anywhere else, and globals are evil, as we know :)
> 
> +1, please pass front-end URL as an explicit parameter.

Removed global variable.

[apavlov, 2013-12-27 14:53:53]

lgtm

[commit-bot: I haz the power, 2013-12-27 14:54:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vsevik@chromium.org/121263002/160001

[commit-bot: I haz the power, 2013-12-27 17:21:18]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-12-30 11:33:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vsevik@chromium.org/121263002/310001

[commit-bot: I haz the power, 2013-12-30 12:54:39]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-12-30 14:19:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vsevik@chromium.org/121263002/480001

[commit-bot: I haz the power, 2013-12-30 15:47:49]

Change committed as 164379