Issue 464293002: [DevTools] ConsoleMessage storage moved from ConsoleAgent

Issue 464293002: [DevTools] ConsoleMessage storage moved from ConsoleAgent (Closed)

Created:
6 years, 4 months ago by kozyatinskiy1

Modified:
6 years, 3 months ago

Reviewers:
vsevik, aandrey, yurys

CC:
blink-reviews, vsevik+blink_chromium.org, caseq+blink_chromium.org, arv+blink, eustas+blink_chromium.org, malch+blink_chromium.org, horo+watch_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, abarth-chromium, loislo+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, blink-reviews-bindings_chromium.org, Inactive, falken, devtools-reviews_chromium.org, kinuko+worker_chromium.org, apavlov+blink_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org, oilpan-reviews

Base URL:
https://chromium.googlesource.com/chromium/blink.git@remove-can-generate

Project:
blink

Visibility:
Public.

More Reviews

Description

[DevTools] ConsoleMessage storage moved from ConsoleAgent Before this patch console agent always stores messages regardless of whether it is ON or not. In this patch there is a new class - the ConsoleMessageStorage that implements the logic of storing messages outside agent. For page this storage locates in top frame console. For worker - in WorkerGlobalScope. BUG=408121 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181083

Patch Set 1 #

Patch Set 2 : Fix line number #

Patch Set 3 : Fix messages with NetworkSource #

Patch Set 4 : Rebased #

Total comments: 28

Patch Set 5 : #

Total comments: 16

Patch Set 6 : #

Total comments: 42

Patch Set 7 : #

Total comments: 10

Patch Set 8 : #

Total comments: 6

Patch Set 9 : #

Total comments: 2

Patch Set 10 : #

Created: 6 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+437 lines, -440 lines)			Patch
M	Source/bindings/core/v8/V8Initializer.cpp	View	1 2 3 4	1 chunk	+5 lines, -1 line	0 comments	Download
M	Source/core/core.gypi	View	1 2 3 4 5 6 7 8	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/frame/FrameConsole.h	View	1 2 3 4 5 6 7 8 9	3 chunks	+6 lines, -0 lines	0 comments	Download
M	Source/core/frame/FrameConsole.cpp	View	1 2 3 4 5 6 7 8	5 chunks	+24 lines, -6 lines	0 comments	Download
M	Source/core/frame/LocalDOMWindow.cpp	View	1 2 3 4 5 6 7 8 9	2 chunks	+2 lines, -0 lines	0 comments	Download
M	Source/core/frame/LocalFrame.cpp	View	1 2 3 4 5 6 7 8 9	2 chunks	+5 lines, -1 line	0 comments	Download
M	Source/core/inspector/CodeGeneratorInstrumentation.py	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/inspector/ConsoleMessage.h	View	1 2 3 4 5 6 7 8	2 chunks	+10 lines, -2 lines	0 comments	Download
M	Source/core/inspector/ConsoleMessage.cpp	View	1 2 3 4 5 6 7 8	5 chunks	+57 lines, -0 lines	0 comments	Download
A	Source/core/inspector/ConsoleMessageStorage.h	View	1 2 3 4 5 6 7 8	1 chunk	+54 lines, -0 lines	0 comments	Download
A	Source/core/inspector/ConsoleMessageStorage.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+85 lines, -0 lines	0 comments	Download
M	Source/core/inspector/InspectorConsoleAgent.h	View	1 2 3 4 5 6	3 chunks	+3 lines, -4 lines	0 comments	Download
M	Source/core/inspector/InspectorConsoleAgent.cpp	View	1 2 3 4 5 6 7	12 chunks	+123 lines, -51 lines	0 comments	Download
D	Source/core/inspector/InspectorConsoleMessage.h	View	1 2 3 4	1 chunk	+0 lines, -91 lines	0 comments	Download
D	Source/core/inspector/InspectorConsoleMessage.cpp	View	1 2 3 4	1 chunk	+0 lines, -261 lines	0 comments	Download
M	Source/core/inspector/InspectorController.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/inspector/PageConsoleAgent.h	View	1 2 3 4 5 6	1 chunk	+9 lines, -3 lines	0 comments	Download
M	Source/core/inspector/PageConsoleAgent.cpp	View	1 2 3 4 5 6	3 chunks	+10 lines, -1 line	0 comments	Download
M	Source/core/inspector/WorkerConsoleAgent.h	View	1 2 3 4 5 6	1 chunk	+10 lines, -3 lines	0 comments	Download
M	Source/core/inspector/WorkerConsoleAgent.cpp	View	1 2 3 4 5 6	2 chunks	+8 lines, -1 line	0 comments	Download
M	Source/core/inspector/WorkerInspectorController.cpp	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/testing/Internals.cpp	View	1 2 3 4 5 6 7 8 9	3 chunks	+6 lines, -6 lines	0 comments	Download
M	Source/core/workers/WorkerConsole.h	View	1 2 3 4 5	2 chunks	+1 line, -1 line	0 comments	Download
M	Source/core/workers/WorkerGlobalScope.h	View	1 2 3 4 5	4 chunks	+5 lines, -1 line	0 comments	Download
M	Source/core/workers/WorkerGlobalScope.cpp	View	1 2 3 4 5 6 7	4 chunks	+8 lines, -1 line	0 comments	Download
M	Source/core/workers/WorkerMessagingProxy.cpp	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 27 (0 generated)

Expand Messages | Collapse Messages

vsevik

https://chromiumcodereview.appspot.com/464293002/diff/80001/Source/core/frame/FrameConsole.cpp File Source/core/frame/FrameConsole.cpp (right): https://chromiumcodereview.appspot.com/464293002/diff/80001/Source/core/frame/FrameConsole.cpp#newcode69 Source/core/frame/FrameConsole.cpp:69: unsigned lineNumber = consoleMessage->lineNumber(); This one is worth a ...

6 years, 4 months ago (2014-08-14 08:06:23 UTC) #2

kozyatinskiy1

https://codereview.chromium.org/464293002/diff/80001/Source/core/frame/FrameConsole.cpp File Source/core/frame/FrameConsole.cpp (right): https://codereview.chromium.org/464293002/diff/80001/Source/core/frame/FrameConsole.cpp#newcode69 Source/core/frame/FrameConsole.cpp:69: unsigned lineNumber = consoleMessage->lineNumber(); On 2014/08/14 08:06:21, vsevik wrote: ...

6 years, 4 months ago (2014-08-20 13:44:19 UTC) #3

vsevik

https://codereview.chromium.org/464293002/diff/160001/Source/core/frame/FrameConsole.h File Source/core/frame/FrameConsole.h (right): https://codereview.chromium.org/464293002/diff/160001/Source/core/frame/FrameConsole.h#newcode34 Source/core/frame/FrameConsole.h:34: #include "core/inspector/ConsoleMessageStorage.h" Is this needed? https://codereview.chromium.org/464293002/diff/160001/Source/core/inspector/ConsoleMessageStorage.cpp File Source/core/inspector/ConsoleMessageStorage.cpp (right): ...

6 years, 4 months ago (2014-08-25 13:04:02 UTC) #4

kozyatinskiy1

6 years, 4 months ago (2014-08-25 14:12:39 UTC) #5

kozyatinskiy1

@yurys, please take a look. I need owner review for Source/bindings/core/v8/V8Initializer.cpp

6 years, 4 months ago (2014-08-25 15:00:11 UTC) #6

yurys

https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp File Source/core/frame/FrameConsole.cpp (right): https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp#newcode138 Source/core/frame/FrameConsole.cpp:138: LocalFrame* localTopFrame = toLocalFrame(curFrame->tree().top()); What if top is not ...

6 years, 3 months ago (2014-08-26 07:38:56 UTC) #7

aandrey

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

6 years, 3 months ago (2014-08-26 08:56:43 UTC) #8

aandrey

https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp File Source/core/frame/FrameConsole.cpp (right): https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp#newcode79 Source/core/frame/FrameConsole.cpp:79: messageStorage()->reportMessage(consoleMessage); why this moved after the if- check? https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp#newcode141 ...

6 years, 3 months ago (2014-08-26 08:56:43 UTC) #9

kozyatinskiy1

https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp File Source/core/frame/FrameConsole.cpp (right): https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp#newcode79 Source/core/frame/FrameConsole.cpp:79: messageStorage()->reportMessage(consoleMessage); On 2014/08/26 08:56:42, aandrey wrote: > why this ...

6 years, 3 months ago (2014-08-26 09:39:30 UTC) #10

aandrey

https://codereview.chromium.org/464293002/diff/180001/Source/core/inspector/ConsoleMessageStorage.cpp File Source/core/inspector/ConsoleMessageStorage.cpp (right): https://codereview.chromium.org/464293002/diff/180001/Source/core/inspector/ConsoleMessageStorage.cpp#newcode34 Source/core/inspector/ConsoleMessageStorage.cpp:34: m_messages.remove(0, expireConsoleMessagesStep); On 2014/08/26 09:39:29, kozyatinskiy wrote: > On ...

6 years, 3 months ago (2014-08-26 10:02:20 UTC) #11

aandrey

https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp File Source/core/frame/FrameConsole.cpp (right): https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp#newcode79 Source/core/frame/FrameConsole.cpp:79: messageStorage()->reportMessage(consoleMessage); On 2014/08/26 09:39:29, kozyatinskiy wrote: > On 2014/08/26 ...

6 years, 3 months ago (2014-08-26 10:12:27 UTC) #12

vsevik

https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp File Source/core/frame/FrameConsole.cpp (right): https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp#newcode79 Source/core/frame/FrameConsole.cpp:79: messageStorage()->reportMessage(consoleMessage); The data that is stored in console message ...

6 years, 3 months ago (2014-08-26 11:44:39 UTC) #13

kozyatinskiy1

https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp File Source/core/frame/FrameConsole.cpp (right): https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/FrameConsole.cpp#newcode79 Source/core/frame/FrameConsole.cpp:79: messageStorage()->reportMessage(consoleMessage); On 2014/08/26 11:44:39, vsevik wrote: > The data ...

6 years, 3 months ago (2014-08-26 12:37:50 UTC) #14

https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/Frame...
File Source/core/frame/FrameConsole.cpp (right):

https://codereview.chromium.org/464293002/diff/180001/Source/core/frame/Frame...
Source/core/frame/FrameConsole.cpp:79:
messageStorage()->reportMessage(consoleMessage);
On 2014/08/26 11:44:39, vsevik wrote:
> The data that is stored in console message has the best possible quality after
> the reportMessage call, so using it is always good, it is not an error.
> The only reason we still have the code above is because we are afraid that the
> too many test expectations might change if we remove it and we want to check
> them carefully (in a separate patch).
> Alexey, would you mind checking if we actually need the if-else statement
above?

Yes, we have 26 devtools tests and I think a lot of other tests that depend on
this. I can remove this if-else in a separate patch.

https://codereview.chromium.org/464293002/diff/180001/Source/core/inspector/C...
File Source/core/inspector/ConsoleMessageStorage.cpp (right):

https://codereview.chromium.org/464293002/diff/180001/Source/core/inspector/C...
Source/core/inspector/ConsoleMessageStorage.cpp:34: m_messages.remove(0,
expireConsoleMessagesStep);
On 2014/08/26 10:02:19, aandrey wrote:
> On 2014/08/26 09:39:29, kozyatinskiy wrote:
> > On 2014/08/26 08:56:42, aandrey wrote:
> > > if expireConsoleMessagesStep const is to improve performance, then
consider
> > > using Deque instead of Vector
> > 
> > Our deque implementation hasn't operation like delete elements in range
> 0..idx,
> > I can call method removeFirst expireConsoleMessagesStep times but I suppose
> that
> > it isn't correct.
> 
> I'd do like this:
> 
> Deque<RefPtr<ConsoleMessage> > m_messages;
> 
> if (m_messages.size() >= maximumConsoleMessages)
>     m_messages.removeFirst();
> m_messages.append(message);
> 
> Once m_messages reaches maximumConsoleMessages capacity, there will be no
> memmove, unlike if using Vector.

In this patch, I just stand the logic of the console agent in the console
message storage. I will change Vector to Deque in separate patch.

vsevik

Looks good, but there some places that still need to be refactored. I'd rather land ...

6 years, 3 months ago (2014-08-27 09:13:21 UTC) #15

kozyatinskiy1

https://codereview.chromium.org/464293002/diff/200001/Source/core/inspector/ConsoleMessageStorage.h File Source/core/inspector/ConsoleMessageStorage.h (right): https://codereview.chromium.org/464293002/diff/200001/Source/core/inspector/ConsoleMessageStorage.h#newcode19 Source/core/inspector/ConsoleMessageStorage.h:19: static PassOwnPtr<ConsoleMessageStorage> create(ExecutionContext* context) On 2014/08/27 09:13:21, vsevik wrote: ...

6 years, 3 months ago (2014-08-27 12:01:27 UTC) #16

yurys

https://codereview.chromium.org/464293002/diff/220001/Source/core/frame/FrameConsole.h File Source/core/frame/FrameConsole.h (right): https://codereview.chromium.org/464293002/diff/220001/Source/core/frame/FrameConsole.h#newcode34 Source/core/frame/FrameConsole.h:34: #include "core/inspector/ConsoleMessageStorage.h" Forward declaration should be enough in the ...

6 years, 3 months ago (2014-08-29 07:40:16 UTC) #18

kozyatinskiy1

6 years, 3 months ago (2014-08-29 08:20:05 UTC) #20

yurys

lgtm https://codereview.chromium.org/464293002/diff/260001/Source/core/frame/FrameConsole.h File Source/core/frame/FrameConsole.h (right): https://codereview.chromium.org/464293002/diff/260001/Source/core/frame/FrameConsole.h#newcode34 Source/core/frame/FrameConsole.h:34: #include "core/inspector/ConsoleMessageStorage.h" This include can be removed now:)

6 years, 3 months ago (2014-08-29 08:22:19 UTC) #21

kozyatinskiy1

https://codereview.chromium.org/464293002/diff/260001/Source/core/frame/FrameConsole.h File Source/core/frame/FrameConsole.h (right): https://codereview.chromium.org/464293002/diff/260001/Source/core/frame/FrameConsole.h#newcode34 Source/core/frame/FrameConsole.h:34: #include "core/inspector/ConsoleMessageStorage.h" On 2014/08/29 08:22:19, yurys wrote: > This ...

6 years, 3 months ago (2014-08-29 08:26:45 UTC) #22

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kozyatinskiy@google.com/464293002/280001

6 years, 3 months ago (2014-08-29 08:27:24 UTC) #24

commit-bot: I haz the power

Committed patchset #10 (id:280001) as 181083

6 years, 3 months ago (2014-08-29 09:38:28 UTC) #25

sof

A couple of CLs involving ConsoleMessage haven't been quite Oilpan compatible - could you Cc: ...

6 years, 3 months ago (2014-08-29 10:03:05 UTC) #26

sof

6 years, 3 months ago (2014-08-29 10:05:13 UTC) #27

Message was sent while issue was closed.

On 2014/08/29 10:03:05, sof wrote:
> A couple of CLs involving ConsoleMessage haven't been quite Oilpan compatible
-
> could you Cc: on any CLs involving further changes involving that class,
please?
> Or any other CLs that appear to use Oilpan types - we're happy to look them
over
> and catch any problems earlier. :)

Cc: oilpan-reviews

Expand Messages | Collapse Messages