Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1890513004:
Remove dependency on the DevTools agent for console logs. (Closed)
|
Created:
4 years, 8 months ago by carlosk Modified:
4 years, 8 months ago CC:
dcheng, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, clamy, creis+watch_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove dependency on the DevTools agent for console logs.
With the upcoming change of having web security related checks made in
the browser process there's the need of sending messages to the affected
frame console. The current implementation depended on the devtools agent
being set for the renderer; when that was not true logs would be dropped
silently. This was the case when running layout tests.
This change removes that dependency so that log requests coming from the
browser process during layout tests are executed.
BUG=576270
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
TBR=nasko@chromium.org
Committed: https://crrev.com/91da0781306a006952e67ee4423a59a5a0da8fb6
Cr-Commit-Position: refs/heads/master@{#389441}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Added new enum to WebConsoleMessage to mirror MessageSource and more. #
Total comments: 8
Patch Set 3 : Changes from dcheng@ comments. #Patch Set 4 : Removed extra Source prefix from strongly typed enum. #Patch Set 5 : Rebase. #
Total comments: 7
Patch Set 6 : Testing not checking for DevTools agent existence before logging to console. #Patch Set 7 : Reuse current console log call but removed devtools agent dependency. #
Dependent Patchsets: Messages
Total messages: 28 (10 generated)
Description was changed from ========== Allows sending security console messages from the browser. With the upcoming change of having web security related checks made in the browser process there's the need of sending security related messages to the affected frame console. This change adds this capability. The main differences between this and the current devtools console message sending is a) this one doesn't depend on the devtools agent being set for the renderer and b) it uses the "security" message source. BUG=576270 ========== to ========== Allows sending security console messages from the browser. With the upcoming change of having web security related checks made in the browser process there's the need of sending security related messages to the affected frame console. This change adds this capability. The main differences between this and the current devtools console message sending is a) this one doesn't depend on the devtools agent being set for the renderer and b) it uses the "security" message source. BUG=576270 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
carlosk@chromium.org changed reviewers: + mkwst@chromium.org, nasko@chromium.org
nasko@, mkwst@: PTAL.
Please introduce the "source" enum in the WebConsoleMessage instead, it should mimic the Blink one. We don't need to copy all the plumbing over and over. Moving handling from devtools_agent seems orthogonal and not necessary as a part of this change.
Thanks. On 2016/04/14 13:09:11, pfeldman_ooo wrote: > Please introduce the "source" enum in the WebConsoleMessage instead, it should > mimic the Blink one. We don't need to copy all the plumbing over and over. I was in doubt about that approach or the one I took. Will change to that one then. > Moving handling from devtools_agent seems orthogonal and not necessary as a part > of this change. It was not orthogonal as that was the way to unify the log level translation to a single place. And the participation of the agent in this logic seemed unnecessary as a) the actual output didn't depend in any way on the agent and b) that was the only caller of that method on the agent. WDYT?
https://codereview.chromium.org/1890513004/diff/1/content/public/browser/rend... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/1890513004/diff/1/content/public/browser/rend... content/public/browser/render_frame_host.h:126: virtual void AddSecurityMessageToConsole(ConsoleMessageLevel level, Why add a public method if it isn't called from outside content/? https://codereview.chromium.org/1890513004/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1890513004/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:797: blink::WebConsoleMessage::Level translateConsoleLevel( Start with capital T, since this is Chromium code. https://codereview.chromium.org/1890513004/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:1825: GetWebFrame()->addSecurityMessageToConsole(wcm); Why call a method? Just use frame_.
Thanks! I addressed current comments but I'm waiting on pfeldman@ reply to decide on reverting devtools_agent changes. https://codereview.chromium.org/1890513004/diff/1/content/public/browser/rend... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/1890513004/diff/1/content/public/browser/rend... content/public/browser/render_frame_host.h:126: virtual void AddSecurityMessageToConsole(ConsoleMessageLevel level, On 2016/04/14 14:14:44, nasko (very slow Apr 8-18) wrote: > Why add a public method if it isn't called from outside content/? Done. https://codereview.chromium.org/1890513004/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1890513004/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:797: blink::WebConsoleMessage::Level translateConsoleLevel( On 2016/04/14 14:14:44, nasko (very slow Apr 8-18) wrote: > Start with capital T, since this is Chromium code. Done. https://codereview.chromium.org/1890513004/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:1825: GetWebFrame()->addSecurityMessageToConsole(wcm); On 2016/04/14 14:14:44, nasko (very slow Apr 8-18) wrote: > Why call a method? Just use frame_. Done.
https://codereview.chromium.org/1890513004/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1890513004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:800: case CONSOLE_MESSAGE_LEVEL_DEBUG: Why can't we just use the blink public API enum directly? Seems kind of silly to duplicate it in content, especially since it gets duplicated again in Blink core. https://codereview.chromium.org/1890513004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1890513004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:774: case WebConsoleMessage::SourceXML: FWIW, the usual idiom is to make the core and web enums match. There's a bunch of static_asserts that guarantee this to be so, and then we just static cast across the boundary. But I guess this is matching the existing code... https://codereview.chromium.org/1890513004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:804: default: That being said, if you keep the switch (though it should be moved to a central place, probably the file that defines WebConsoleMessage::Source), remove the default case since this will keep the compiler from emitting errors for unhandled enum values. Same with the previous switch. https://codereview.chromium.org/1890513004/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebConsoleMessage.h (right): https://codereview.chromium.org/1890513004/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebConsoleMessage.h:49: enum Source { enum class Source.
Thanks dcheng@. This is my 1st Blink CL and I don't know the usual practices so I'm mostly mimicking what's already in place. https://codereview.chromium.org/1890513004/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1890513004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:800: case CONSOLE_MESSAGE_LEVEL_DEBUG: On 2016/04/14 17:29:11, dcheng wrote: > Why can't we just use the blink public API enum directly? Seems kind of silly to > duplicate it in content, especially since it gets duplicated again in Blink > core. I am unable to answer that question as I'm not aware of the current practices. But for this enum there is a secondary reason for its existence: it's used to generate equivalent constant Java code [1]. And there are calls from Java into this console API [2] so this can't be changed. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com... [2] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... https://codereview.chromium.org/1890513004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1890513004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:774: case WebConsoleMessage::SourceXML: On 2016/04/14 17:29:11, dcheng wrote: > FWIW, the usual idiom is to make the core and web enums match. There's a bunch > of static_asserts that guarantee this to be so, and then we just static cast > across the boundary. But I guess this is matching the existing code... Static casting was I wanted to do initially but again I'm just matching what's already in place. If it is acceptable I'll be glad to change both switch-case blocks here. And only now I found (and added) the value checking static asserts in AssertMatchingEnums.cpp. https://codereview.chromium.org/1890513004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:804: default: On 2016/04/14 17:29:11, dcheng wrote: > That being said, if you keep the switch (though it should be moved to a central > place, probably the file that defines WebConsoleMessage::Source), My rule of thumb is to create a central, shared method when there's more than one use of it which is not the case here. If pfeldman@ confirms he still thinks I should re-add the devtools_agent method than that would be the case and I will do so. WDYT? Note: that method can't be created in WebConsoleMessage because code in public/ can't import from core/ [1]. > remove the > default case since this will keep the compiler from emitting errors for > unhandled enum values. Same with the previous switch. Done. I coupled that with setting a default for the output value to avoid undefined behavior. Notice this changes the existing behavior for unsupported values from a crash/ignore to having a log. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1890513004/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebConsoleMessage.h (right): https://codereview.chromium.org/1890513004/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebConsoleMessage.h:49: enum Source { On 2016/04/14 17:29:11, dcheng wrote: > enum class Source. Done.
mkwst@, pfeldman@ and dcheng@: can I have an update from you so that I can move forward with this CL?
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/1890513004/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1890513004/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:359: void RenderFrameHostImpl::AddSecurityMessageToConsole( We already have one above. https://codereview.chromium.org/1890513004/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1890513004/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:130: void AddMessageToConsole(ConsoleMessageLevel level, We already have one. https://codereview.chromium.org/1890513004/diff/80001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/1890513004/diff/80001/content/common/frame_me... content/common/frame_messages.h:673: IPC_MESSAGE_ROUTED2(FrameMsg_AddSecurityMessageToConsole, ditto https://codereview.chromium.org/1890513004/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1890513004/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:2388: if (devtools_agent_) { This was already always true, but now you don't even use it below. https://codereview.chromium.org/1890513004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1890513004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:753: MessageSource webCoreMessageSource = OtherMessageSource; You have static assert, you should be fine casting.
I know we already had a console log IPC and I mentioned the differences between that one and this new one in the change description. But it is not true that the DevTools agent would always be set. I was using that console log IPC initially and would have layout test failures because the agent was not set and log calls would simply be dropped. I was initially just keeping the current functionality intact. So to try and reuse the existing IPC as pfeldman@ suggested and as it was previously stated that setting MessageSource to "Security" is not important, I removed the dependency on the existence of the devtools agent. It solves my issue and did not cause any test failures. https://codereview.chromium.org/1890513004/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1890513004/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:2388: if (devtools_agent_) { On 2016/04/21 21:48:30, pfeldman wrote: > This was already always true, but now you don't even use it below. As explained, this was not always true, for instance, whne running layout tests. https://codereview.chromium.org/1890513004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1890513004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:753: MessageSource webCoreMessageSource = OtherMessageSource; On 2016/04/21 21:48:30, pfeldman wrote: > You have static assert, you should be fine casting. This code was fully removed.
Description was changed from ========== Allows sending security console messages from the browser. With the upcoming change of having web security related checks made in the browser process there's the need of sending security related messages to the affected frame console. This change adds this capability. The main differences between this and the current devtools console message sending is a) this one doesn't depend on the devtools agent being set for the renderer and b) it uses the "security" message source. BUG=576270 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove dependency on the DevTools agent for console logs. With the upcoming change of having web security related checks made in the browser process there's the need of sending messages to the affected frame console. The current implementation depended on the devtools agent being set for the renderer; when that was not true logs would be dropped silently. This was the case when running layout tests. This change removes that dependency so that log requests coming from the browser process during layout tests are executed. BUG=576270 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Remove dependency on the DevTools agent for console logs. With the upcoming change of having web security related checks made in the browser process there's the need of sending messages to the affected frame console. The current implementation depended on the devtools agent being set for the renderer; when that was not true logs would be dropped silently. This was the case when running layout tests. This change removes that dependency so that log requests coming from the browser process during layout tests are executed. BUG=576270 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove dependency on the DevTools agent for console logs. With the upcoming change of having web security related checks made in the browser process there's the need of sending messages to the affected frame console. The current implementation depended on the devtools agent being set for the renderer; when that was not true logs would be dropped silently. This was the case when running layout tests. This change removes that dependency so that log requests coming from the browser process during layout tests are executed. BUG=576270 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
lgtm. it seems like oopif made devtools_agent conditional and did not resolve this console plumbing. are you following up with the security level?
On 2016/04/22 17:16:07, pfeldman wrote: > lgtm. it seems like oopif made devtools_agent conditional and did not resolve > this console plumbing. are you following up with the security level? Thanks. When you say "security level" you mean setting the message source to SecurityMessageSource? Someone else I talked to (that I don't happen to recall who is) mentioned that wasn't important, hence my rolling back the changes in regards to that. If that is required then I'd prefer to make that change in this CL.
Right, the source. No, I don't think it is required at this point. If we ever want to treat security source differently, we can always introduce it.
So feel free to land this. Since you only touched a comment in the messages file, you should be fine TBR-ing security folks.
Description was changed from ========== Remove dependency on the DevTools agent for console logs. With the upcoming change of having web security related checks made in the browser process there's the need of sending messages to the affected frame console. The current implementation depended on the devtools agent being set for the renderer; when that was not true logs would be dropped silently. This was the case when running layout tests. This change removes that dependency so that log requests coming from the browser process during layout tests are executed. BUG=576270 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove dependency on the DevTools agent for console logs. With the upcoming change of having web security related checks made in the browser process there's the need of sending messages to the affected frame console. The current implementation depended on the devtools agent being set for the renderer; when that was not true logs would be dropped silently. This was the case when running layout tests. This change removes that dependency so that log requests coming from the browser process during layout tests are executed. BUG=576270 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation TBR=nasko@chromium.org ==========
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1890513004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890513004/120001
Message was sent while issue was closed.
Description was changed from ========== Remove dependency on the DevTools agent for console logs. With the upcoming change of having web security related checks made in the browser process there's the need of sending messages to the affected frame console. The current implementation depended on the devtools agent being set for the renderer; when that was not true logs would be dropped silently. This was the case when running layout tests. This change removes that dependency so that log requests coming from the browser process during layout tests are executed. BUG=576270 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation TBR=nasko@chromium.org ========== to ========== Remove dependency on the DevTools agent for console logs. With the upcoming change of having web security related checks made in the browser process there's the need of sending messages to the affected frame console. The current implementation depended on the devtools agent being set for the renderer; when that was not true logs would be dropped silently. This was the case when running layout tests. This change removes that dependency so that log requests coming from the browser process during layout tests are executed. BUG=576270 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation TBR=nasko@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Remove dependency on the DevTools agent for console logs. With the upcoming change of having web security related checks made in the browser process there's the need of sending messages to the affected frame console. The current implementation depended on the devtools agent being set for the renderer; when that was not true logs would be dropped silently. This was the case when running layout tests. This change removes that dependency so that log requests coming from the browser process during layout tests are executed. BUG=576270 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation TBR=nasko@chromium.org ========== to ========== Remove dependency on the DevTools agent for console logs. With the upcoming change of having web security related checks made in the browser process there's the need of sending messages to the affected frame console. The current implementation depended on the devtools agent being set for the renderer; when that was not true logs would be dropped silently. This was the case when running layout tests. This change removes that dependency so that log requests coming from the browser process during layout tests are executed. BUG=576270 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation TBR=nasko@chromium.org Committed: https://crrev.com/91da0781306a006952e67ee4423a59a5a0da8fb6 Cr-Commit-Position: refs/heads/master@{#389441} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/91da0781306a006952e67ee4423a59a5a0da8fb6 Cr-Commit-Position: refs/heads/master@{#389441}
Message was sent while issue was closed.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
Message was sent while issue was closed.
ipc changes lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
