DevTools: simplify CSS agent's setActiveStyleSheets method

DevTools: simplify CSS agent's setActiveStyleSheets method

First of all: below is a postmortem of the behavior, which happens in the bug.

The exception happens because one of the matched style rules refers
to the stylesheet which has never been reported to the front-end.

Stylesheets are reported by the CSS.setActiveStyleSheets method. Interestingly,
the CSS.setActiveStyleSheets method reports only a subset of stylesheets as "added".
For example, the method doesn't report those style sheets which
have already been "bound". Since getMatchedStylesForNode binds stylesheets while
building a response message, a race condition occurs.

-- END POSTMORTEM --

The whole logic of determining which stylesheets are added and removed was
introduced in https://codereview.chromium.org/14320027. Back in the days,
the bound stylesheets were (incorrectly) used as a source of "reported" stylesheets.
Nowadays, thanks to the m_documentToCSSStyleSheets, we have relevant
information of reported stylesheets.

This patch removes all the tricks around reporting stylesheets to the front-end, since
they are not needed any more.

BUG=681492
R=dgozman

Review-Url: https://codereview.chromium.org/2653643005
Cr-Commit-Position: refs/heads/master@{#446303}
Committed: https://chromium.googlesource.com/chromium/src/+/e612b6edf5092c860e19014c7aa700ee2ec7bbbe

[lushnikov, 2017-01-24 12:22:22]

Description was changed from

==========
DevTools: simplify CSS agent's setActiveStyleSheets method

BUG=681492
R=dgozman
==========

to

==========
DevTools: simplify CSS agent's setActiveStyleSheets method

First of all: below is a postmortem of the behavior, which happens in the bug.

The exception happens because one of the matched style rules refers
to the stylesheet which has never been reported to the front-end.

Stylesheets are reported by the CSS.setActiveStyleSheets method. Interestingly,
the CSS.setActiveStyleSheets method reports only a subset of stylesheets as
"added".
For example, the method doesn't report those style sheets which
have already been "bound". Since getMatchedStylesForNode binds stylesheets while
building a response message, a race condition occurs.

-- END POSTMORTEM --

The whole logic of determining which stylesheets are added and removed was
introduced in https://codereview.chromium.org/14320027. Back in the days,
the bound stylesheets were (incorrectly) used as a source of "reported"
stylesheets.
Nowadays, thanks to the m_documentToCSSStyleSheets, we have relevant
information of reported stylesheets.

BUG=681492
R=dgozman
==========

[lushnikov, 2017-01-24 12:22:36]

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

[commit-bot: I haz the power, 2017-01-24 12:22:47]

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

[lushnikov, 2017-01-24 12:23:21]

Description was changed from

==========
DevTools: simplify CSS agent's setActiveStyleSheets method

First of all: below is a postmortem of the behavior, which happens in the bug.

The exception happens because one of the matched style rules refers
to the stylesheet which has never been reported to the front-end.

Stylesheets are reported by the CSS.setActiveStyleSheets method. Interestingly,
the CSS.setActiveStyleSheets method reports only a subset of stylesheets as
"added".
For example, the method doesn't report those style sheets which
have already been "bound". Since getMatchedStylesForNode binds stylesheets while
building a response message, a race condition occurs.

-- END POSTMORTEM --

The whole logic of determining which stylesheets are added and removed was
introduced in https://codereview.chromium.org/14320027. Back in the days,
the bound stylesheets were (incorrectly) used as a source of "reported"
stylesheets.
Nowadays, thanks to the m_documentToCSSStyleSheets, we have relevant
information of reported stylesheets.

BUG=681492
R=dgozman
==========

to

==========
DevTools: simplify CSS agent's setActiveStyleSheets method

First of all: below is a postmortem of the behavior, which happens in the bug.

The exception happens because one of the matched style rules refers
to the stylesheet which has never been reported to the front-end.

Stylesheets are reported by the CSS.setActiveStyleSheets method. Interestingly,
the CSS.setActiveStyleSheets method reports only a subset of stylesheets as
"added".
For example, the method doesn't report those style sheets which
have already been "bound". Since getMatchedStylesForNode binds stylesheets while
building a response message, a race condition occurs.

-- END POSTMORTEM --

The whole logic of determining which stylesheets are added and removed was
introduced in https://codereview.chromium.org/14320027. Back in the days,
the bound stylesheets were (incorrectly) used as a source of "reported"
stylesheets.
Nowadays, thanks to the m_documentToCSSStyleSheets, we have relevant
information of reported stylesheets.

This patch removes all the tricks around reporting stylesheets to the front-end,
since
they are not needed any more.

BUG=681492
R=dgozman
==========

[lushnikov, 2017-01-24 12:23:38]

please, take a look!

[commit-bot: I haz the power, 2017-01-24 12:56:36]

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

[commit-bot: I haz the power, 2017-01-24 12:56:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[pfeldman, 2017-01-24 16:06:37]

You are rather reverting https://codereview.chromium.org/121263002, not the
change you refer to. Could you explain why that was wrong and whether you
refresh styles on front-end open now?

[lushnikov, 2017-01-24 23:44:01]

On 2017/01/24 16:06:37, pfeldman wrote:
> You are rather reverting https://codereview.chromium.org/121263002, not the
> change you refer to. Could you explain why that was wrong and whether you
> refresh styles on front-end open now?

Since now we track exactly which stylesheets has been reported to the front-end
via the m_documentToCSSStyleSheets,
this whole thing is not needed any more. Or am I missing something? Could you
point me to the scenario which
is not covered? (all tests pass, and visually all works fine)

[pfeldman, 2017-01-25 20:16:43]

I'm specifically interested if you are regressing what the change that I
mentioned was fixing.

[lushnikov, 2017-01-26 01:46:16]

On 2017/01/25 20:16:43, pfeldman wrote:
> I'm specifically interested if you are regressing what the change that I
> mentioned was fixing.

I don't understand what that CL is doing - it has unclear description and tricky
code,
but the test it adds still passes.

[pfeldman, 2017-01-26 01:47:15]

"Do not force style sheets update on inspector start." sounds pretty clear to
me.

[pfeldman, 2017-01-26 01:55:13]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2017-01-26 01:55:14]

lgtm

https://codereview.chromium.org/2653643005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (left):

https://codereview.chromium.org/2653643005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:807: if
(isInitialFrontendLoad)
Ok, reading out loud here. isInitialFrontendLoad is only true when I come from
wasEnabled. But in that case removedSheets is always empty, so this branch never
executes anyways.

https://codereview.chromium.org/2653643005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:822: if
(frontend() && !isInitialFrontendLoad)
Again, if we are here, !isInitialFrontendLoad is true.

https://codereview.chromium.org/2653643005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:828: bool isNew =
isInitialFrontendLoad ||
Replace it with
DCHECK(!m_cssStyleSheetToInspectorStyleSheet.contains(cssStyleSheet))

[lushnikov, 2017-01-26 01:59:37]

https://codereview.chromium.org/2653643005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (left):

https://codereview.chromium.org/2653643005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:828: bool isNew =
isInitialFrontendLoad ||
On 2017/01/26 01:55:14, pfeldman wrote:
> Replace it with
> DCHECK(!m_cssStyleSheetToInspectorStyleSheet.contains(cssStyleSheet))

This actually can happen - the getMachedStyles might've bound the style sheet
already, that's what happens in the bug

[lushnikov, 2017-01-26 02:10:53]

The CQ bit was checked by lushnikov@chromium.org

[commit-bot: I haz the power, 2017-01-26 02:11:36]

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, 2017-01-26 03:45:22]

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

[commit-bot: I haz the power, 2017-01-26 03:45:23]

Failed to apply patch for
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:778
  error: third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp: patch
does not apply

Patch:       third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp
Index: third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp
diff --git a/third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp
b/third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp
index
fa2948f303090f5aac181974465ddf10d1f30191..052753b922274f9317e5111c45cf8f1375847505
100644
--- a/third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp
+++ b/third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp
@@ -699,7 +699,7 @@ void InspectorCSSAgent::flushPendingProtocolNotifications()
{
   HeapHashSet<Member<Document>> invalidatedDocuments;
   m_invalidatedDocuments.swap(invalidatedDocuments);
   for (Document* document : invalidatedDocuments)
-    updateActiveStyleSheets(document, ExistingFrontendRefresh);
+    updateActiveStyleSheets(document);
 }
 
 void InspectorCSSAgent::reset() {
@@ -745,7 +745,7 @@ void InspectorCSSAgent::wasEnabled() {
   m_domAgent->setDOMListener(this);
   HeapVector<Member<Document>> documents = m_domAgent->documents();
   for (Document* document : documents)
-    updateActiveStyleSheets(document, InitialFrontendLoad);
+    updateActiveStyleSheets(document);
 }
 
 Response InspectorCSSAgent::disable() {
@@ -778,20 +778,15 @@ void InspectorCSSAgent::activeStyleSheetsUpdated(Document*
document) {
   m_invalidatedDocuments.add(document);
 }
 
-void InspectorCSSAgent::updateActiveStyleSheets(
-    Document* document,
-    StyleSheetsUpdateType styleSheetsUpdateType) {
+void InspectorCSSAgent::updateActiveStyleSheets(Document* document) {
   HeapVector<Member<CSSStyleSheet>> newSheetsVector;
   InspectorCSSAgent::collectAllDocumentStyleSheets(document, newSheetsVector);
-  setActiveStyleSheets(document, newSheetsVector, styleSheetsUpdateType);
+  setActiveStyleSheets(document, newSheetsVector);
 }
 
 void InspectorCSSAgent::setActiveStyleSheets(
     Document* document,
-    const HeapVector<Member<CSSStyleSheet>>& allSheetsVector,
-    StyleSheetsUpdateType styleSheetsUpdateType) {
-  bool isInitialFrontendLoad = styleSheetsUpdateType == InitialFrontendLoad;
-
+    const HeapVector<Member<CSSStyleSheet>>& allSheetsVector) {
   HeapHashSet<Member<CSSStyleSheet>>* documentCSSStyleSheets =
       m_documentToCSSStyleSheets.get(document);
   if (!documentCSSStyleSheets) {
@@ -804,8 +799,6 @@ void InspectorCSSAgent::setActiveStyleSheets(
   for (CSSStyleSheet* cssStyleSheet : allSheetsVector) {
     if (removedSheets.contains(cssStyleSheet)) {
       removedSheets.remove(cssStyleSheet);
-      if (isInitialFrontendLoad)
-        addedSheets.push_back(cssStyleSheet);
     } else {
       addedSheets.push_back(cssStyleSheet);
     }
@@ -819,21 +812,18 @@ void InspectorCSSAgent::setActiveStyleSheets(
     documentCSSStyleSheets->remove(cssStyleSheet);
     if (m_idToInspectorStyleSheet.contains(inspectorStyleSheet->id())) {
       String id = unbindStyleSheet(inspectorStyleSheet);
-      if (frontend() && !isInitialFrontendLoad)
+      if (frontend())
         frontend()->styleSheetRemoved(id);
     }
   }
 
   for (CSSStyleSheet* cssStyleSheet : addedSheets) {
-    bool isNew = isInitialFrontendLoad ||
-                 !m_cssStyleSheetToInspectorStyleSheet.contains(cssStyleSheet);
-    if (isNew) {
       InspectorStyleSheet* newStyleSheet = bindStyleSheet(cssStyleSheet);
       documentCSSStyleSheets->add(cssStyleSheet);
-      if (frontend())
+      if (frontend()) {
         frontend()->styleSheetAdded(
             newStyleSheet->buildObjectForStyleSheetInfo());
-    }
+      }
   }
 
   if (documentCSSStyleSheets->isEmpty())
@@ -842,8 +832,7 @@ void InspectorCSSAgent::setActiveStyleSheets(
 
 void InspectorCSSAgent::documentDetached(Document* document) {
   m_invalidatedDocuments.remove(document);
-  setActiveStyleSheets(document, HeapVector<Member<CSSStyleSheet>>(),
-                       ExistingFrontendRefresh);
+  setActiveStyleSheets(document, HeapVector<Member<CSSStyleSheet>>());
 }
 
 bool InspectorCSSAgent::forcePseudoState(Element* element,
@@ -1508,7 +1497,7 @@ Response InspectorCSSAgent::createStyleSheet(
   if (!inspectorStyleSheet)
     return Response::Error("No target stylesheet found");
 
-  updateActiveStyleSheets(document, ExistingFrontendRefresh);
+  updateActiveStyleSheets(document);
 
   *outStyleSheetId = inspectorStyleSheet->id();
   return Response::OK();

[lushnikov, 2017-01-26 08:52:03]

The CQ bit was checked by lushnikov@chromium.org

[lushnikov, 2017-01-26 08:52:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2653643005/#ps40001
(title: "rebaseline")

[commit-bot: I haz the power, 2017-01-26 08:52:18]

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, 2017-01-26 10:10:59]

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

[commit-bot: I haz the power, 2017-01-26 10:11:00]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lushnikov, 2017-01-26 10:18:00]

The CQ bit was checked by lushnikov@chromium.org

[commit-bot: I haz the power, 2017-01-26 10:18:13]

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, 2017-01-26 11:48:47]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485425880081980,
"parent_rev": "4bc8ee0bc2934699e0bed53f43f74166e1998185", "commit_rev":
"e612b6edf5092c860e19014c7aa700ee2ec7bbbe"}

[commit-bot: I haz the power, 2017-01-26 11:49:46]

Description was changed from

==========
DevTools: simplify CSS agent's setActiveStyleSheets method

First of all: below is a postmortem of the behavior, which happens in the bug.

The exception happens because one of the matched style rules refers
to the stylesheet which has never been reported to the front-end.

Stylesheets are reported by the CSS.setActiveStyleSheets method. Interestingly,
the CSS.setActiveStyleSheets method reports only a subset of stylesheets as
"added".
For example, the method doesn't report those style sheets which
have already been "bound". Since getMatchedStylesForNode binds stylesheets while
building a response message, a race condition occurs.

-- END POSTMORTEM --

The whole logic of determining which stylesheets are added and removed was
introduced in https://codereview.chromium.org/14320027. Back in the days,
the bound stylesheets were (incorrectly) used as a source of "reported"
stylesheets.
Nowadays, thanks to the m_documentToCSSStyleSheets, we have relevant
information of reported stylesheets.

This patch removes all the tricks around reporting stylesheets to the front-end,
since
they are not needed any more.

BUG=681492
R=dgozman
==========

to

==========
DevTools: simplify CSS agent's setActiveStyleSheets method

First of all: below is a postmortem of the behavior, which happens in the bug.

The exception happens because one of the matched style rules refers
to the stylesheet which has never been reported to the front-end.

Stylesheets are reported by the CSS.setActiveStyleSheets method. Interestingly,
the CSS.setActiveStyleSheets method reports only a subset of stylesheets as
"added".
For example, the method doesn't report those style sheets which
have already been "bound". Since getMatchedStylesForNode binds stylesheets while
building a response message, a race condition occurs.

-- END POSTMORTEM --

The whole logic of determining which stylesheets are added and removed was
introduced in https://codereview.chromium.org/14320027. Back in the days,
the bound stylesheets were (incorrectly) used as a source of "reported"
stylesheets.
Nowadays, thanks to the m_documentToCSSStyleSheets, we have relevant
information of reported stylesheets.

This patch removes all the tricks around reporting stylesheets to the front-end,
since
they are not needed any more.

BUG=681492
R=dgozman

Review-Url: https://codereview.chromium.org/2653643005
Cr-Commit-Position: refs/heads/master@{#446303}
Committed:
https://chromium.googlesource.com/chromium/src/+/e612b6edf5092c860e19014c7aa7...
==========

[commit-bot: I haz the power, 2017-01-26 11:49:48]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e612b6edf5092c860e19014c7aa7...