DevTools: automatic console message grouping changes: [chromium/src : master]

0 views
Skip to first unread message

Pavel Feldman (Gerrit)

unread,
Oct 22, 2017, 2:29:49 AM10/22/17
to apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Erik Luo, Dmitry Gozman, Andrey Lushnikov, chromium...@chromium.org, devtools...@chromium.org

This change is ready for review.

View Change

    To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: master
    Gerrit-MessageType: comment
    Gerrit-Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
    Gerrit-Change-Number: 731969
    Gerrit-PatchSet: 1
    Gerrit-Owner: Pavel Feldman <pfel...@chromium.org>
    Gerrit-Reviewer: Andrey Lushnikov <lush...@chromium.org>
    Gerrit-Reviewer: Dmitry Gozman <dgo...@chromium.org>
    Gerrit-Reviewer: Erik Luo <lu...@chromium.org>
    Gerrit-Reviewer: Pavel Feldman <pfel...@chromium.org>
    Gerrit-Comment-Date: Sun, 22 Oct 2017 06:29:41 +0000
    Gerrit-HasComments: No
    Gerrit-HasLabels: No

    Blaise Bruer (Gerrit)

    unread,
    Oct 22, 2017, 7:24:08 PM10/22/17
    to Pavel Feldman, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Commit Bot, Erik Luo, Dmitry Gozman, Andrey Lushnikov, chromium...@chromium.org, devtools...@chromium.org

    View Change

    17 comments:

    To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: master
    Gerrit-MessageType: comment
    Gerrit-Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
    Gerrit-Change-Number: 731969
    Gerrit-PatchSet: 2
    Gerrit-Owner: Pavel Feldman <pfel...@chromium.org>
    Gerrit-Reviewer: Andrey Lushnikov <lush...@chromium.org>
    Gerrit-Reviewer: Dmitry Gozman <dgo...@chromium.org>
    Gerrit-Reviewer: Erik Luo <lu...@chromium.org>
    Gerrit-Reviewer: Pavel Feldman <pfel...@chromium.org>
    Gerrit-CC: Blaise Bruer <all...@chromium.org>
    Gerrit-CC: Commit Bot <commi...@chromium.org>
    Gerrit-Comment-Date: Sun, 22 Oct 2017 23:24:03 +0000
    Gerrit-HasComments: Yes
    Gerrit-HasLabels: No

    Andrey Lushnikov (Gerrit)

    unread,
    Oct 22, 2017, 7:38:42 PM10/22/17
    to Pavel Feldman, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Commit Bot, Erik Luo, Dmitry Gozman, chromium...@chromium.org, devtools...@chromium.org

    View Change

    2 comments:

    To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: master
    Gerrit-MessageType: comment
    Gerrit-Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
    Gerrit-Change-Number: 731969
    Gerrit-PatchSet: 2
    Gerrit-Owner: Pavel Feldman <pfel...@chromium.org>
    Gerrit-Reviewer: Andrey Lushnikov <lush...@chromium.org>
    Gerrit-Reviewer: Dmitry Gozman <dgo...@chromium.org>
    Gerrit-Reviewer: Erik Luo <lu...@chromium.org>
    Gerrit-Reviewer: Pavel Feldman <pfel...@chromium.org>
    Gerrit-CC: Blaise Bruer <all...@chromium.org>
    Gerrit-CC: Commit Bot <commi...@chromium.org>
    Gerrit-Comment-Date: Sun, 22 Oct 2017 23:38:37 +0000
    Gerrit-HasComments: Yes
    Gerrit-HasLabels: No

    Andrey Lushnikov (Gerrit)

    unread,
    Oct 22, 2017, 7:49:22 PM10/22/17
    to Pavel Feldman, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Commit Bot, Erik Luo, Dmitry Gozman, chromium...@chromium.org, devtools...@chromium.org

    Patch set 2:Code-Review +1

    View Change

    1 comment:

      • since this is to avoid nasty exception, let's put a TODO here so that future readers won't be confused.

    To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: master
    Gerrit-MessageType: comment
    Gerrit-Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
    Gerrit-Change-Number: 731969
    Gerrit-PatchSet: 2
    Gerrit-Owner: Pavel Feldman <pfel...@chromium.org>
    Gerrit-Reviewer: Andrey Lushnikov <lush...@chromium.org>
    Gerrit-Reviewer: Dmitry Gozman <dgo...@chromium.org>
    Gerrit-Reviewer: Erik Luo <lu...@chromium.org>
    Gerrit-Reviewer: Pavel Feldman <pfel...@chromium.org>
    Gerrit-CC: Blaise Bruer <all...@chromium.org>
    Gerrit-CC: Commit Bot <commi...@chromium.org>
    Gerrit-Comment-Date: Sun, 22 Oct 2017 23:49:17 +0000
    Gerrit-HasComments: Yes
    Gerrit-HasLabels: Yes

    Dmitry Gozman (Gerrit)

    unread,
    Oct 22, 2017, 10:19:03 PM10/22/17
    to Pavel Feldman, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Andrey Lushnikov, Commit Bot, Erik Luo, chromium...@chromium.org, devtools...@chromium.org

    Played with this:

    • Grouping inside groups confuses me - it's kind of similar, but not quite. See, for example, "[Violation] Avoid using document.write()." on sfgate.com.
    • If I hit preserve log, reload the page, and clear console while reloading, I sometimes get twice as many items in a group. I suspect the old ones are not cleared out?

    View Change

    2 comments:

    To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: master
    Gerrit-MessageType: comment
    Gerrit-Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
    Gerrit-Change-Number: 731969
    Gerrit-PatchSet: 2
    Gerrit-Owner: Pavel Feldman <pfel...@chromium.org>
    Gerrit-Reviewer: Andrey Lushnikov <lush...@chromium.org>
    Gerrit-Reviewer: Dmitry Gozman <dgo...@chromium.org>
    Gerrit-Reviewer: Erik Luo <lu...@chromium.org>
    Gerrit-Reviewer: Pavel Feldman <pfel...@chromium.org>
    Gerrit-CC: Blaise Bruer <all...@chromium.org>
    Gerrit-CC: Commit Bot <commi...@chromium.org>
    Gerrit-Comment-Date: Mon, 23 Oct 2017 02:18:51 +0000
    Gerrit-HasComments: Yes
    Gerrit-HasLabels: No

    Dmitry Gozman (Gerrit)

    unread,
    Oct 22, 2017, 10:21:56 PM10/22/17
    to Pavel Feldman, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Andrey Lushnikov, Commit Bot, Erik Luo, chromium...@chromium.org, devtools...@chromium.org

    More feedback:

    • Clicking "Group similar" does not regroup things.
    • (Not sure it's related), grouping on sfgate.com, then typing "url" in Filter bar makes DevTools unresponsive.

    View Change

      To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: comment
      Gerrit-Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
      Gerrit-Change-Number: 731969
      Gerrit-PatchSet: 2
      Gerrit-Owner: Pavel Feldman <pfel...@chromium.org>
      Gerrit-Reviewer: Andrey Lushnikov <lush...@chromium.org>
      Gerrit-Reviewer: Dmitry Gozman <dgo...@chromium.org>
      Gerrit-Reviewer: Erik Luo <lu...@chromium.org>
      Gerrit-Reviewer: Pavel Feldman <pfel...@chromium.org>
      Gerrit-CC: Blaise Bruer <all...@chromium.org>
      Gerrit-CC: Commit Bot <commi...@chromium.org>
      Gerrit-Comment-Date: Mon, 23 Oct 2017 02:21:51 +0000
      Gerrit-HasComments: No
      Gerrit-HasLabels: No

      Erik Luo (Gerrit)

      unread,
      Oct 22, 2017, 10:24:40 PM10/22/17
      to Pavel Feldman, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Dmitry Gozman, Andrey Lushnikov, Commit Bot, chromium...@chromium.org, devtools...@chromium.org

      Un-checking and re-checking 'Group similar' box sometimes gives me large blank gaps in console viewport :(

      View Change

      5 comments:

        • We use `%o` as a substitution parameter from the backend. Perhaps the regex can be `/ [%]o/g` to ensure we don't match on a '%o' that happens to be inside a user's URL.

      To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: comment
      Gerrit-Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
      Gerrit-Change-Number: 731969
      Gerrit-PatchSet: 2
      Gerrit-Owner: Pavel Feldman <pfel...@chromium.org>
      Gerrit-Reviewer: Andrey Lushnikov <lush...@chromium.org>
      Gerrit-Reviewer: Dmitry Gozman <dgo...@chromium.org>
      Gerrit-Reviewer: Erik Luo <lu...@chromium.org>
      Gerrit-Reviewer: Pavel Feldman <pfel...@chromium.org>
      Gerrit-CC: Blaise Bruer <all...@chromium.org>
      Gerrit-CC: Commit Bot <commi...@chromium.org>
      Gerrit-Comment-Date: Mon, 23 Oct 2017 02:24:35 +0000
      Gerrit-HasComments: Yes
      Gerrit-HasLabels: No

      Pavel Feldman (Gerrit)

      unread,
      Oct 23, 2017, 2:54:54 AM10/23/17
      to apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, maki mukai, Erik Luo, Dmitry Gozman, Andrey Lushnikov, Commit Bot, chromium...@chromium.org, devtools...@chromium.org

      View Change

      24 comments:

        • I'll make viewMessage non-null.

        • Same as above?

        • Patch Set #2, Line 718: this._consoleMessages[i].setInSimilarGroup(false);

          Can we move this into a listener when the setting changes? This appears to be a hot path.

        • It is not too hot - one need to expand the group and have element in the tree for serious overhead.

        • Patch Set #2, Line 745: if (!viewMessagesInGroupArray.find(x => this._shouldMessageBeVisible(x)))

          this can be cached per group; otherwise, if all messages fall into the same group, it will be N^2?

        • If all the messages are in the same group, we only get here once.

        • Patch Set #2, Line 745: x => this._shouldMessageBeVisible(x)

          nit: this._shouldMessageBeVisible. […]

          I now use arrows when I can - it reads better.

        • When a group contains N repeats, they are collapsed into the first occurring viewMessage. […]

          :( I see. This is getting too complicated, I'll need to follow up.

        • Can we cleanly not have it create this marker in two different places?

        • Why not?

        • This function is going to get ran quite a bit, can we shortcut it to 1357 if we have the types and r […]

          Done (it was taking 2 microseconds)

        • Simplified would be: /\/[\w\. […]

          This has not changed.

        • I don't understand what 'pathLine' is or why it needs to end with ':\d+'? Maybe we should remove thi […]

          This has not changed.

        • Done

        • Done

        • Do we really want to force ourselves to keep updating this every year? This means within 2 years we' […]

          Yey! We'll celebrate Chrome 80 with updating it in a couple of years!

        • It might be a good idea to consider ensuring there is a spacelike character on each side of this, ot […]

          Done

        • Ack

        • Could we also add a handlerRegex? I think it'd be nice in a demo to group these: […]

          Let's follow up. I haven't seen them trigger, so did not spot them.

        • We cache the console view message itself.

        • Lets extract this named types into an enum. Then we can do something like: […]

          That's too generic to my taste.

        • Done

        • If we set the string to `Common. […]

          Ack

        • Patch Set #2, Line 151: if (firstActiveIndex <= i && i - firstActiveIndex < this._renderedItems.length && i <= lastActiveIndex)

          Hmm... Are you breaking the invariant now? This worked previously. […]

        • I'm not breaking the invariant. This code was broken:

          invalidate() is doing
          1. this._itemCount = this._provider.itemCount()
          2. this._rebuildCumulativeHeights

          (2) fails because itemCount is out of sync with this._renderedItems.

          It never worked...

      To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: comment
      Gerrit-Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
      Gerrit-Change-Number: 731969
      Gerrit-PatchSet: 2
      Gerrit-Owner: Pavel Feldman <pfel...@chromium.org>
      Gerrit-Reviewer: Andrey Lushnikov <lush...@chromium.org>
      Gerrit-Reviewer: Dmitry Gozman <dgo...@chromium.org>
      Gerrit-Reviewer: Erik Luo <lu...@chromium.org>
      Gerrit-Reviewer: Pavel Feldman <pfel...@chromium.org>
      Gerrit-CC: Blaise Bruer <all...@chromium.org>
      Gerrit-CC: Commit Bot <commi...@chromium.org>
      Gerrit-CC: maki mukai <yyymak...@gmail.com>
      Gerrit-Comment-Date: Mon, 23 Oct 2017 06:54:49 +0000
      Gerrit-HasComments: Yes
      Gerrit-HasLabels: No

      Pavel Feldman (Gerrit)

      unread,
      Oct 23, 2017, 2:57:48 AM10/23/17
      to apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, maki mukai, Erik Luo, Dmitry Gozman, Andrey Lushnikov, Commit Bot, chromium...@chromium.org, devtools...@chromium.org
      • Grouping inside groups confuses me - it's kind of similar, but not quite. See, for example, "[Violation] Avoid using document.write()." on sfgate.com.

      I know, but I don't want to ungroup repeat count - things might explode!

      • If I hit preserve log, reload the page, and clear console while reloading, I sometimes get twice as many items in a group. I suspect the old ones are not cleared out?

      I can't reproduce this.

      • Clicking "Group similar" does not regroup things.

      It does for me.

      • (Not sure it's related), grouping on sfgate.com, then typing "url" in Filter bar makes DevTools unresponsive.

      That's the deep text content I mentioned at the meeting.

      View Change

        To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.

        Gerrit-Project: chromium/src
        Gerrit-Branch: master
        Gerrit-MessageType: comment
        Gerrit-Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
        Gerrit-Change-Number: 731969
        Gerrit-PatchSet: 2
        Gerrit-Owner: Pavel Feldman <pfel...@chromium.org>
        Gerrit-Reviewer: Andrey Lushnikov <lush...@chromium.org>
        Gerrit-Reviewer: Dmitry Gozman <dgo...@chromium.org>
        Gerrit-Reviewer: Erik Luo <lu...@chromium.org>
        Gerrit-Reviewer: Pavel Feldman <pfel...@chromium.org>
        Gerrit-CC: Blaise Bruer <all...@chromium.org>
        Gerrit-CC: Commit Bot <commi...@chromium.org>
        Gerrit-CC: maki mukai <yyymak...@gmail.com>
        Gerrit-Comment-Date: Mon, 23 Oct 2017 06:57:43 +0000
        Gerrit-HasComments: No
        Gerrit-HasLabels: No

        Pavel Feldman (Gerrit)

        unread,
        Oct 23, 2017, 3:03:12 AM10/23/17
        to apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, maki mukai, Erik Luo, Dmitry Gozman, Andrey Lushnikov, Commit Bot, chromium...@chromium.org, devtools...@chromium.org

        Patch set 3:Commit-Queue +2

        View Change

          To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.

          Gerrit-Project: chromium/src
          Gerrit-Branch: master
          Gerrit-MessageType: comment
          Gerrit-Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
          Gerrit-Change-Number: 731969
          Gerrit-PatchSet: 3
          Gerrit-Owner: Pavel Feldman <pfel...@chromium.org>
          Gerrit-Reviewer: Andrey Lushnikov <lush...@chromium.org>
          Gerrit-Reviewer: Dmitry Gozman <dgo...@chromium.org>
          Gerrit-Reviewer: Erik Luo <lu...@chromium.org>
          Gerrit-Reviewer: Pavel Feldman <pfel...@chromium.org>
          Gerrit-CC: Blaise Bruer <all...@chromium.org>
          Gerrit-CC: Commit Bot <commi...@chromium.org>
          Gerrit-CC: maki mukai <yyymak...@gmail.com>
          Gerrit-Comment-Date: Mon, 23 Oct 2017 07:03:07 +0000
          Gerrit-HasComments: No
          Gerrit-HasLabels: Yes

          Commit Bot (Gerrit)

          unread,
          Oct 23, 2017, 3:03:46 AM10/23/17
          to Pavel Feldman, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, maki mukai, Erik Luo, Dmitry Gozman, Andrey Lushnikov, chromium...@chromium.org, devtools...@chromium.org

          CQ is trying da patch.

          Note: The patchset sent to CQ was uploaded after this CL was approved.
          "review comments addressed" https://chromium-review.googlesource.com/c/731969/3

          Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/731969/3

          Bot data: {"action": "start", "triggered_at": "2017-10-23T07:03:07.0Z", "cq_cfg_revision": "573b6c39d3de239a70e8fa672647b63bf0bd1f89", "revision": "63303d27747d9ba3affe8c28905fc22e1f20e4e9"}

          Gerrit-Comment-Date: Mon, 23 Oct 2017 07:03:42 +0000
          Gerrit-HasComments: No
          Gerrit-HasLabels: No

          Commit Bot (Gerrit)

          unread,
          Oct 23, 2017, 4:45:08 AM10/23/17
          to Pavel Feldman, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, maki mukai, Erik Luo, Dmitry Gozman, Andrey Lushnikov, chromium...@chromium.org, devtools...@chromium.org

          Commit Bot merged this change.

          View Change

          Approvals: Andrey Lushnikov: Looks good to me Pavel Feldman: Commit
          DevTools: automatic console message grouping changes:

          - New message matching logic
          - Setting in the toolbar
          - Nesting marker
          - Bugfixes

          Bug: 774694
          Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
          Reviewed-on: https://chromium-review.googlesource.com/731969
          Commit-Queue: Pavel Feldman <pfel...@chromium.org>
          Reviewed-by: Andrey Lushnikov <lush...@chromium.org>
          Cr-Commit-Position: refs/heads/master@{#510737}
          ---
          M third_party/WebKit/LayoutTests/http/tests/devtools/console-xhr-logging.js
          M third_party/WebKit/LayoutTests/http/tests/devtools/console/console-timestamp.js
          M third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
          M third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
          M third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js
          M third_party/WebKit/Source/devtools/front_end/console/consoleView.css
          M third_party/WebKit/Source/devtools/front_end/console/module.json
          M third_party/WebKit/Source/devtools/front_end/console_model/ConsoleModel.js
          M third_party/WebKit/Source/devtools/front_end/main/Main.js
          9 files changed, 183 insertions(+), 64 deletions(-)


          To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.

          Gerrit-Project: chromium/src
          Gerrit-Branch: master
          Gerrit-MessageType: merged
          Gerrit-Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
          Gerrit-Change-Number: 731969
          Gerrit-PatchSet: 4
          Gerrit-Owner: Pavel Feldman <pfel...@chromium.org>
          Gerrit-Reviewer: Andrey Lushnikov <lush...@chromium.org>
          Gerrit-Reviewer: Commit Bot <commi...@chromium.org>
          Gerrit-Reviewer: Dmitry Gozman <dgo...@chromium.org>
          Gerrit-Reviewer: Erik Luo <lu...@chromium.org>
          Gerrit-Reviewer: Pavel Feldman <pfel...@chromium.org>
          Gerrit-CC: Blaise Bruer <all...@chromium.org>
          Reply all
          Reply to author
          Forward
          0 new messages